pharo-project
There should not be statements after a return statement. This is a parse error.
Can we have more info on what is the problem here? I did not find this kind of code in the image so maybe it's to say we should add warnings but there are there already
What is the change expected here?
As an advocate of a strict mode, this crosses the line of the warning towards a compiler error. Particularly for newcomers.
There is no practical reason to let this pass. The unreachable statement will never be reached, so it cannot even trigger a runtime error. Unless, this method compiles to a method that throws an exception on entry, which is not the case.
I've seen people using this as a mechanism to comment code. Please, if you wanted to comment code, use comments.
Love, Guille
Just FYI, in Pharo11 this was a compilation error. This behavior changed in Pharo12.
The following test is not green so this is plain bad.
testReturnShouldBeTheLastStatement
| ast |
ast := self parserClass parseMethod:
'tmp ^ 42. 33 + 33'.
self assert: ast isFaulty
To me this is a parser error and I do not know when the compiler is actually adding errors.
parseStatementInto: statementList periodList: periods withAcceptedStatementClosers: aCollectionOfClosers
"Parse a statement and store it into the statement list.
Parse statements as valid only if they end in one of the accepted statement closers provided by the enclosing scope.
E.g., if we are parsing a block, a valid closer is ].
Put any period found at the end of this statement int the periods collection.
"
| returnPosition node startOfStatementToken err |
"Record the token found at the beginning of the statement.
If at the end we did not consume it, consume it and mark it as a bad statement for now"
startOfStatementToken := currentToken.
"If the current statement starts with ^, parse it as a return + an expression.
Otherwise just parse it as an expression.
Expressions start with assignments, which have lower precedence"
(currentToken isSpecial: $^)
ifTrue: [
returnPosition := currentToken start.
self step.
node := self returnNodeClass
start: returnPosition
value: self parseAssignment ]
ifFalse: [ node := self parseAssignment ].
"If what follows is a closer that do not match the current scope, consume them and produce the corresponding errors wrapping the statement."
[(')]}' includes: currentToken value)
and: [ (aCollectionOfClosers includes: currentToken value) not ]]
whileTrue: [
| contents |
err := self parseErrorNode: 'Missing opener for closer: ', currentToken value asString.
"If the parsing failed because it did not match anything, there was nothing before the unexpected closer"
contents := startOfStatementToken = currentToken ifTrue: [ #() ] ifFalse: [ { node } ].
node := (OCUnfinishedStatementErrorNode from: err contents: contents)
valueAfter: currentToken value asString;
stop: currentToken stop.
self step ].
"If the parsing failed because it did not match anything, consume the current token and continue parsing."
startOfStatementToken = currentToken ifTrue: [
"We did not progress, its mean that 1. we have a error node and 2. we are missing something (or found something unexpected, its the same thing)."
"Solution to recover. Consume the token to create the statement."
node := OCParseErrorNode new
errorMessage: 'Unexpected token';
value: currentToken value asString;
start: currentToken start;
stop: currentToken stop;
errorPosition: currentToken start.
self step ].
"Next we should have the end of the statement
- a dot or
- an accepted scope closer e.g., )]} or
- the end of the stream.
If it is neither, we should mark our statement as unfinished statement.
And we do not stack unfinished statements."
(currentToken value ~= $.
and: [ (aCollectionOfClosers includes: currentToken value) not
and: [ currentToken isEOF not
and: [ node isUnfinishedStatement not ] ] ] ) ifTrue: [
"I do not like the following guard. If currentToken is an unrelated error,
then parserError will unfortunately consume and lose it.
A better design is needed."
node isParseError ifFalse: [
err := self parseErrorNode: 'End of statement expected'.
node := OCUnfinishedStatementErrorNode from: err contents: { node } ]
].
"Then consume all dots and comments coming after this well formed statement, if there was any"
(currentToken isSpecial: $.) ifTrue: [
periods add: currentToken start.
self step.
self addCommentsTo: node ].
[ currentToken isSpecial: $. ]
whileTrue: [
periods add: currentToken start.
self step ].
"If the previous node in the statement list is a return node, mark this node as unreachable"
(statementList notEmpty and: [ statementList last isReturn ]) ifTrue: [
node addWarning: 'Unreachable statement' ].
"Commit the statement to the statements list"
statementList add: node
So
(statementList notEmpty and: [ statementList last isReturn ]) ifTrue: [
node addWarning: 'Unreachable statement' ].
this should raise an error not a warning.
testReturnShouldBeTheLastStatement
self
should: [ self parserClass parseMethod:
'tmp ^ 42. 33 + 33' ]
raise: Error.
parseStatementInto: statementList periodList: periods withAcceptedStatementClosers: aCollectionOfClosers
"Parse a statement and store it into the statement list.
Parse statements as valid only if they end in one of the accepted statement closers provided by the enclosing scope.
E.g., if we are parsing a block, a valid closer is ].
Put any period found at the end of this statement int the periods collection.
"
| returnPosition node startOfStatementToken err |
"Record the token found at the beginning of the statement.
If at the end we did not consume it, consume it and mark it as a bad statement for now"
startOfStatementToken := currentToken.
"If the current statement starts with ^, parse it as a return + an expression.
Otherwise just parse it as an expression.
Expressions start with assignments, which have lower precedence"
(currentToken isSpecial: $^)
ifTrue: [
returnPosition := currentToken start.
self step.
node := self returnNodeClass
start: returnPosition
value: self parseAssignment ]
ifFalse: [ node := self parseAssignment ].
"If what follows is a closer that do not match the current scope, consume them and produce the corresponding errors wrapping the statement."
[(')]}' includes: currentToken value)
and: [ (aCollectionOfClosers includes: currentToken value) not ]]
whileTrue: [
| contents |
err := self parseErrorNode: 'Missing opener for closer: ', currentToken value asString.
"If the parsing failed because it did not match anything, there was nothing before the unexpected closer"
contents := startOfStatementToken = currentToken ifTrue: [ #() ] ifFalse: [ { node } ].
node := (OCUnfinishedStatementErrorNode from: err contents: contents)
valueAfter: currentToken value asString;
stop: currentToken stop.
self step ].
"If the parsing failed because it did not match anything, consume the current token and continue parsing."
startOfStatementToken = currentToken ifTrue: [
"We did not progress, its mean that 1. we have a error node and 2. we are missing something (or found something unexpected, its the same thing)."
"Solution to recover. Consume the token to create the statement."
node := OCParseErrorNode new
errorMessage: 'Unexpected token';
value: currentToken value asString;
start: currentToken start;
stop: currentToken stop;
errorPosition: currentToken start.
self step ].
"Next we should have the end of the statement
- a dot or
- an accepted scope closer e.g., )]} or
- the end of the stream.
If it is neither, we should mark our statement as unfinished statement.
And we do not stack unfinished statements."
(currentToken value ~= $.
and: [ (aCollectionOfClosers includes: currentToken value) not
and: [ currentToken isEOF not
and: [ node isUnfinishedStatement not ] ] ] ) ifTrue: [
"I do not like the following guard. If currentToken is an unrelated error,
then parserError will unfortunately consume and lose it.
A better design is needed."
node isParseError ifFalse: [
err := self parseErrorNode: 'End of statement expected'.
node := OCUnfinishedStatementErrorNode from: err contents: { node } ]
].
"Then consume all dots and comments coming after this well formed statement, if there was any"
(currentToken isSpecial: $.) ifTrue: [
periods add: currentToken start.
self step.
self addCommentsTo: node ].
[ currentToken isSpecial: $. ]
whileTrue: [
periods add: currentToken start.
self step ].
"If the previous node in the statement list is a return node, mark this node as unreachable"
(statementList notEmpty and: [ statementList last isReturn ]) ifTrue: [
self error: 'after the return statement is an Unreachable statement' ].
"Commit the statement to the statements list"
statementList add: node
a simple way would be to not tag this as a Warning, but and Error:
Go to OCParser>>#parseStatementInto:periodList:withAcceptedStatementClosers:
there you find at the end:
"If the previous node in the statement list is a return node, mark this node as unreachable"
(statementList notEmpty and: [ statementList last isReturn ]) ifTrue: [
node addWarning: 'Unreachable statement' ].
Change the #addWarning: to #addError:, in the latest image there is a halt in the inform:, remove that and the behavior is:
- inform: shows the : 'Unreachable statement’
- source is not accepted, but it stays in non-accepted form
- with showing in the gutter Error correctly, too.
This change does not touch how comments are handled, this means still add a comment after the return as before.
This needs in addtion fixes to the tests, as the Warning behavior is tested, see
OCCodeSnippet class>>#badSimpleExpressions
For info: I made a PR to suggest a way to manage the inform in the core packages of Pharo.
The Idea is to raise a InformativeNotication. It's superclass will print the message in the transcript or stdout. But if NewTools is present, it will use the #inform: of NewTools :)
Like this, the compiler can provide informations without relying on the UIManager>>#inform:
The PR has not yet been reviewed I think so I'm not yet sure this will get integrated. But that if it is, then we can easily remove all the inform: from Pharo