Compiler gets permitFaulty#13084
Merged
MarcusDenker merged 11 commits intopharo-project:Pharo12from Mar 21, 2023
Merged
Conversation
…gs, but it's sill bad
Closed
Contributor
Author
|
All tests passed except 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:) isFaultybut the failed assert is 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 isFaultyand the issue is in 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 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 ].
^ nilExcept that, for some reason, the kind is now Therefore, it now has to do codeForNoSource
"if everything fails, decompile the bytecode"
"If there is no compiler, we cannot decompile it"
Smalltalk hasCompiler ifFalse: [ ^ nil ].
^(self compiler decompileMethod: self) formattedCodebut method
3 + (RuntimeSyntaxError signalSyntaxError:
'<an unprintable nonliteral value>')That is completely legal and valid Pharo code, thus is considered a non-faulty method. So:
|
…at was removed by mistake
Contributor
Author
|
All green, but review (and merge) #13080 first |
MarcusDenker
approved these changes
Mar 21, 2023
78 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
permitFaultyis 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:
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.