Skip to content

Compiler gets permitFaulty#13084

Merged
MarcusDenker merged 11 commits intopharo-project:Pharo12from
privat:faulty-code-permit-faulty
Mar 21, 2023
Merged

Compiler gets permitFaulty#13084
MarcusDenker merged 11 commits intopharo-project:Pharo12from
privat:faulty-code-permit-faulty

Conversation

@privat
Copy link
Contributor

@privat privat commented Mar 20, 2023

This is a rewrite of #13048 and includes code of #13080 that is not merged yet (so bigger commit list and diff)

Parsing and compiling in a "faulty" way is not really a preference toggle, it's mainly the responsibility of client to state what they want and what kind of results they are ready to deal with.

So this PR stop relying on the options #optionParseErrorsNonInteractiveOnly #optionParseErrors #optionSkipSemanticWarnings and propose a single compilation flag permitFaulty.
This simplifies the code of clients and the internal logic of the compiler.

Also, I did not like the options: way because the syntax is cumbersome, long names are easy to misspell, and spelling errors are silently ignored.
Note that the 3 options are now unused. I did not remove or deprecate them because doing so break the image at startup. Maybe something related to the system settings, but I'm not sure. (see https://ci.inria.fr/pharo-ci-jenkins2/blue/organizations/jenkins/Test%20pending%20pull%20request%20and%20branch%20Pipeline/detail/PR-13048/1/pipeline if you can help me with that)

The flag permitFaulty is stored in the compilerContext but accessible from the Compiler (I'm not a fan or having two classes is not the concern of the PR).

The main difference with #13048 is that there is a new default policy:

  • for parsing and semantic analysis, permitFaulty is true by default.
  • for compiling and evaluating the flag is false by default.

This simple policy is consistent with the new parser default policy (cf #13080) but also consistent with the usage, since I removed a lot of options: in clients that wanted only an AST in silence. A looked at every sender of parse, and I did not find a single client, except some tests, that required a non-faulty parsing and semantic analysis.

@privat privat mentioned this pull request Mar 20, 2023
@privat
Copy link
Contributor Author

privat commented Mar 21, 2023

All tests passed except CompiledMethodTest>>#testIsFaulty.

The thing is that this test goes by:

testIsFaulty
	|  cm |
	cm := OpalCompiler new
				source: 'method 3+';
				permitFaulty: true;
				compile.
	self assert: cm isFaulty.
	self deny: (OCASTTranslator>>#visitParseErrorNode:) isFaulty

but the failed assert is CompiledMethod>>#isFaulty that goes by:

isFaulty
 	"check if this method was compiled from syntactically wrong code"
	| ast |
	"fast pre-check: all methods with syntax errors reference this symbol"
  	(self refersToLiteral: #signalSyntaxError:) ifFalse: [ ^false].
	"we have to parse the ast here as #ast does not know that this needs optionParseErrors"
	ast := self methodClass compiler
				source: self sourceCode;
				parse.
	^ ast isFaulty

and the issue is in CompiledMethod>>#sourceCode that goes by:

sourceCode
	"Retrieve or reconstruct the source code for this method."

	| trailer |
	trailer := self trailer.
	trailer sourceCode ifNotNil: [:code | ^ code ].
	trailer hasSourcePointer ifFalse: [ ^ self codeForNoSource ].

	^ self getSourceFromFile ifEmpty: [ self codeForNoSource ]

And CompiledMethodTrailer>>#sourceCode that goes by

sourceCode
	"Answer the source code of compiled method as it was decoded.
	Note: it does not attempts to read from source files using sourcePointer,
	nor reconstruct the source code using temp names"

	kind == #EmbeddedSource ifTrue: [ ^data ].
	kind == #EmbeddedSourceWide ifTrue: [ ^data ].
	kind == #EmbeddedSourceQCompress ifTrue: [ ^data ].
	kind == #EmbeddedSourceZip ifTrue: [ ^data ].
	^ nil

Except that, for some reason, the kind is now #NoTrailer and previously it was #EmbeddedSource.

Therefore, it now has to do CompiledMethod>>#codeForNoSource that goes by:

codeForNoSource
	"if everything fails, decompile the bytecode"
	"If there is no compiler, we cannot decompile it"
	Smalltalk hasCompiler ifFalse: [ ^ nil ].

	 ^(self compiler decompileMethod: self) formattedCode

but (self compiler decompileMethod: self) formattedCode return the following:

method
	3 + (RuntimeSyntaxError signalSyntaxError:
		 '<an unprintable nonliteral value>')

That is completely legal and valid Pharo code, thus is considered a non-faulty method.

So:

  • CompiledMethod>>#isFaulty seems not that reliable.
  • I was responsible because in my precipitation to clean options, I removed an optionEmbeddSources in the failing test... oops...

@privat
Copy link
Contributor Author

privat commented Mar 21, 2023

All green, but review (and merge) #13080 first

@MarcusDenker MarcusDenker merged commit ff0db23 into pharo-project:Pharo12 Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants