Improve faulty compilation - unify handling of undeclared variables

src/Kernel-Tests-Extended/CompiledMethodTest.class.st

@@ -794,6 +794,150 @@ CompiledMethodTest >> testSendsSelector [
 	self deny: ((CompiledCode >> #sendsSelector:) sendsSelector: #doBreakfastForMe)
 ]
 
+{ #category : #'tests - literals' }
+CompiledMethodTest >> testUndeclaredReparationWithClass [
+
+	| method c c2 |
+
+	Smalltalk globals removeKey: #TestUndeclaredVariable ifAbsent: [].
+	Undeclared removeKey: #TestUndeclaredVariable ifAbsent: []. "Because obsolete may remain"
+
+	method := self class compiler compile: 'x ^TestUndeclaredVariable ifNil: [ TestUndeclaredVariable ]'.
+	self assert: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: nil.
+
+	"Adding a class DOES repair"
+	c := Object subclass: #TestUndeclaredVariable.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: c.
+
+	"Removal of class DOES undeclare users"
+	c removeFromSystem.
+	self assert: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: c.
+
+	"Thus, adding back repair DOES repair again"
+	c2 := Object subclass: #TestUndeclaredVariable.
+	self deny: c equals: c2.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: c2.
+
+	"cleanup"
+	Smalltalk globals removeKey: #TestUndeclaredVariable
+]
+
+{ #category : #'tests - literals' }
+CompiledMethodTest >> testUndeclaredReparationWithGlobal [
+
+	| method |
+
+	Smalltalk globals removeKey: #TestUndeclaredVariable ifAbsent: [].
+
+	method := self class compiler compile: 'x ^TestUndeclaredVariable ifNil: [ TestUndeclaredVariable ]'.
+	self assert: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: nil.
+
+	"Adding a global DOES repair"
+	Smalltalk globals at: #TestUndeclaredVariable put: 42.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"Removal of globals DOES NOT undeclare users"
+	Smalltalk globals removeKey: #TestUndeclaredVariable.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"Thus, adding back DOES NOT repair"
+	Smalltalk globals at: #TestUndeclaredVariable put: 421.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"cleanup"
+	Smalltalk globals removeKey: #TestUndeclaredVariable
+]
+
+{ #category : #'tests - literals' }
+CompiledMethodTest >> testUndeclaredReparationWithInstanceVariable [
+
+	| method method2 method3 method4 class receiver |
+
+	Smalltalk globals at: #TestUndeclaredVariableClass ifPresent: [ :c | c removeFromSystem ].
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: '' category: ''.
+	self assert: (class instVarIndexFor: #TestUndeclaredVariable) equals: 0.
+	receiver := class new.
+
+	"Note: unlike related tests, we need here a reveiver (see above) and to install the method in the class"
+	method := class >> (class compile: 'x ^TestUndeclaredVariable ifNil: [ TestUndeclaredVariable ]').
+	self assert: method usesUndeclareds.
+	self assert: (receiver executeMethod: method) equals: nil.
+
+	"Update the class (same identity, not a new class)"
+	self assert: (Object subclass: #TestUndeclaredVariableClass instanceVariableNames: 'TestUndeclaredVariable' classVariableNames: '' category: '') equals: class.
+	self assert: (class instVarIndexFor: #TestUndeclaredVariable) equals: 1.
+	receiver instVarAt: 1 put: 42.
+
+	"Adding ivars DOES not repair the CompiledMethod"
+	self assert: method usesUndeclareds.
+	"But recompile and install a NEW method"
+	self deny: method equals: (method2 := class>>#x). "It is a new method"
+	self deny: method2 usesUndeclareds.
+	self assert: (receiver executeMethod: method2) equals: 42.
+
+	"Removal of ivar DOES recompile a new new one"
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: '' category: ''.
+	self deny: method2 equals: (method3 := class>>#x). "It is a new method"
+	self assert: method3 usesUndeclareds.
+	self assert: (receiver executeMethod: method3) equals: nil.
+
+	"Thus adding back DOES recompile again"
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: 'TestUndeclaredVariable' classVariableNames: '' category: ''.
+	receiver instVarAt: 1 put: 421.
+	self deny: method3 equals: (method4 := class>>#x). "It is a new method"
+	self deny: method4 usesUndeclareds.
+	self assert: (receiver executeMethod: method4) equals: 421.
+
+	"cleanup"
+	class removeFromSystem
+]
+
+{ #category : #'tests - literals' }
+CompiledMethodTest >> testUndeclaredReparationWithShared [
+
+	| method class |
+
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: '' category: ''.
+	self assert: (class bindingOf: #TestUndeclaredVariable) isNil.
+
+	method := class compiler compile: 'x ^TestUndeclaredVariable ifNil: [ TestUndeclaredVariable ]'.
+	self assert: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: nil.
+
+	"Add and initialize a shared"
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: 'TestUndeclaredVariable' category: ''.
+	(class bindingOf: #TestUndeclaredVariable) value: 42.
+
+	"Method is repaired"
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"Remove shared from class"
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: '' category: ''.
+	self assert: (class bindingOf: #TestUndeclaredVariable) isNil.
+
+	"Removal of shared DOES NOT undeclare users"
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"Thus, adding back DOES NOT repair"
+	class := Object subclass: #TestUndeclaredVariableClass instanceVariableNames: '' classVariableNames: 'TestUndeclaredVariable' category: ''.
+	(class bindingOf: #TestUndeclaredVariable) value: 421.
+	self deny: method usesUndeclareds.
+	self assert: (nil executeMethod: method) equals: 42.
+
+	"cleanup"
+	class removeFromSystem
+]
+
 { #category : #'tests - testing' }
 CompiledMethodTest >> testUsesUndeclareds [
 	| method |

src/Kernel/UndeclaredVariable.class.st

@@ -12,7 +12,10 @@ Class {
 }
 
 { #category : #'instance creation' }
-UndeclaredVariable class >> registeredWithName: aString [
+UndeclaredVariable class >> possiblyRegisteredWithName: aString [
+
+	"Get a registered undeclared variable if any, or a new unregistered undeclared variable"
+
 	| varName |
 	varName := aString asSymbol.
 
@@ -23,9 +26,16 @@ UndeclaredVariable class >> registeredWithName: aString [
 			found becomeForward: (self named: varName)].
 		^found	].
 
-	^(self named: varName)
-		register;
-		yourself
+	^self named: varName
+]
+
+{ #category : #'instance creation' }
+UndeclaredVariable class >> registeredWithName: aString [
+
+	| var |
+	var := self possiblyRegisteredWithName: aString.
+	(var isUndeclaredVariable and: [ var isRegistered not ]) ifTrue: [ var register ].
+	^ var
 ]
 
 { #category : #queries }

src/OpalCompiler-Core/OCASTSemanticAnalyzer.class.st

@@ -8,7 +8,8 @@ Class {
 	#instVars : [
 		'scope',
 		'compilationContext',
-		'blockCounter'
+		'blockCounter',
+		'undeclared'
 	],
 	#category : #'OpalCompiler-Core-Semantics'
 }
@@ -140,14 +141,28 @@ OCASTSemanticAnalyzer >> storeIntoReservedVariable: variableNode [
 		signal
 ]
 
+{ #category : #accessing }
+OCASTSemanticAnalyzer >> undeclared [
+
+	^ undeclared ifNil: [ undeclared := Dictionary new ]
+]
+
 { #category : #'error handling' }
 OCASTSemanticAnalyzer >> undeclaredVariable: variableNode [
+	| varName var |
 	variableNode addWarning: 'Undeclared variable'.
-	compilationContext optionSkipSemanticWarnings
-		ifTrue: [ ^UndeclaredVariable named: variableNode name asSymbol ].
+
+	"If a registered undeclared variable exists, use it. Otherwise create an unregistered one."
+	"It will be registered only at backend when a CompiledMethod is produced.
+	Or never if compilation is aborted or if only frontend is requested."
+	varName := variableNode name asSymbol.
+	var := self undeclared at: varName ifAbsentPut: [ UndeclaredVariable possiblyRegisteredWithName: varName ].
+	compilationContext optionSkipSemanticWarnings ifTrue: [ ^ var ].
+
 	^ OCUndeclaredVariableWarning new
 		node: variableNode;
 		compilationContext: compilationContext;
+		variable: var;
 		signal
 ]
 

src/OpalCompiler-Core/OCASTTranslator.class.st

@@ -504,10 +504,16 @@ OCASTTranslator >> visitArrayNode: anArrayNode [
 { #category : #'visitor - double dispatching' }
 OCASTTranslator >> visitAssignmentNode: anAssignmentNode [
 
+	| var |
+	var := anAssignmentNode variable variable.
 	valueTranslator visitNode: anAssignmentNode value.
 	"Even in faulty mode, do not baspass the isWritable semantic to avoid very bad consequences on the image"
-	anAssignmentNode variable variable isWritable ifFalse: [ ^ self emitRuntimeError: anAssignmentNode variable ].
-	anAssignmentNode variable variable emitStore: methodBuilder
+	var isWritable ifFalse: [ ^ self emitRuntimeError: anAssignmentNode variable ].
+
+	"Because we are producing a CompiledMethod, register undeclared variables to `Undeclared`"
+	var isUndeclaredVariable ifTrue: [ var register ].
+
+	var emitStore: methodBuilder
 ]
 
 { #category : #'visitor - double dispatching' }
@@ -700,5 +706,9 @@ OCASTTranslator >> visitSequenceNode: aSequenceNode [
 
 { #category : #'visitor - double dispatching' }
 OCASTTranslator >> visitVariableNode: aVariableNode [
+
+	"Because we are producing a CompiledMethod, register undeclared variables to `Undeclared`"
+	aVariableNode variable isUndeclaredVariable ifTrue: [ aVariableNode variable register ].
+
 	aVariableNode variable emitValue: methodBuilder
 ]

src/OpalCompiler-Core/OCUndeclaredVariableWarning.class.st

@@ -4,6 +4,9 @@ I get signalled when a temporary variable is used that is not defined.  My defau
 Class {
 	#name : #OCUndeclaredVariableWarning,
 	#superclass : #OCSemanticWarning,
+	#instVars : [
+		'variable'
+	],
 	#category : #'OpalCompiler-Core-Exception'
 }
 
@@ -70,7 +73,7 @@ OCUndeclaredVariableWarning >> declareTempAndPaste: name [
 { #category : #correcting }
 OCUndeclaredVariableWarning >> declareUndefined [
 
-	^UndeclaredVariable registeredWithName: node name
+	^ variable
 ]
 
 { #category : #correcting }
@@ -241,3 +244,15 @@ OCUndeclaredVariableWarning >> substituteWord: correctWord wordInterval: spot of
 
 	^ o + correctWord size - spot size
 ]
+
+{ #category : #accessing }
+OCUndeclaredVariableWarning >> variable [
+
+	^ variable
+]
+
+{ #category : #accessing }
+OCUndeclaredVariableWarning >> variable: anObject [
+
+	variable := anObject
+]

src/OpalCompiler-Tests/OCCompilerTest.class.st

@@ -435,6 +435,53 @@ OCCompilerTest >> testTraitTempShadowing [
 	self assert: warningCount equals: 3
 ]
 
+{ #category : #tests }
+OCCompilerTest >> testUndefinedVariable [
+	"This test shows that undefined variables behave as globals (for now) and is independent of the compilation mode"
+
+	Undeclared removeKey: #undefinedName123 ifAbsent: [ ].
+	self
+		assert: {
+			OpalCompiler new evaluate: 'undefinedName123'.
+			OpalCompiler new evaluate: 'undefinedName123 := 1. undefinedName123'.
+			OpalCompiler new evaluate: 'undefinedName123'.
+			}
+		equals: { nil. 1. 1. }.
+
+	Undeclared removeKey: #undefinedName123 ifAbsent: [ ].
+	self
+		assert: {
+			OpalCompiler new
+				options: #( + optionSkipSemanticWarnings );
+				evaluate: 'undefinedName123'.
+			OpalCompiler new
+				options: #( + optionSkipSemanticWarnings );
+				evaluate: 'undefinedName123 := 2. undefinedName123'.
+			OpalCompiler new
+				options: #( + optionSkipSemanticWarnings );
+				evaluate: 'undefinedName123' }
+		equals: { nil. 2. 2. }.
+
+	"Cleanup"
+	Undeclared removeKey: #undefinedName123 ifAbsent: [ ]
+]
+
+{ #category : #tests }
+OCCompilerTest >> testUndefinedVariableFrontend [
+	"This test shows that undefined variables are registered only on backend"
+
+	Undeclared removeKey: #undefinedName123 ifAbsent: [ ].
+	OpalCompiler new parse: 'foo ^undefinedName123'.
+	self deny: (Undeclared includesKey: #undefinedName123).
+	self should: [ OpalCompiler new compile: 'foo ^undefinedName123 ¿ 2' ] raise: SyntaxErrorNotification.
+	self deny: (Undeclared includesKey: #undefinedName123).
+	OpalCompiler new compile: 'foo ^undefinedName123'.
+	self assert: (Undeclared includesKey: #undefinedName123).
+
+	"Cleanup"
+	Undeclared removeKey: #undefinedName123 ifAbsent: [ ]
+]
+
 { #category : #mocking }
 OCCompilerTest >> text [
 	^ text