Fix diagnostic of ternary operator missing only colon #1677
Conversation
| if let previousTokenKind = currentToken.cursor.previousTokenKind, | ||
| self.at(TokenSpec(previousTokenKind)) |
There was a problem hiding this comment.
This will only catch cases where the two tokens that should be separated by the colon are of the same kind. It won’t catch cases like foo ? 1 someVar.
But actually, I’m thinking there’s no reason to ever set rhs to a MissingExprSyntax (maybe there never was, I don’t know) and just always setting it to nil seems like the cleaner fix to me.
There was a problem hiding this comment.
Oh, right!
I just checked the behavior of the Swift Compliler in Xcode and it doesn't seem to check for expressions that should be after the colon when there is no colon.
One difference is that the Xcode Swift Compiler does not suggest adding an expression in the following cases 🤔
I think the behavior of SwiftSyntax is more accurate in this case, so I'll keep it.
There was a problem hiding this comment.
In addition, in the case below, we need to check the type of both expressions of the colon, which is not possible with just the tokenKind check.
(In my previous commit, I had included logic to check if the tokenKinds were the same in an attempt to solve this problem.)
Is there an already implemented function to check for type mismatches?
I looked through the code, but didn't see anything that checks for type.
There was a problem hiding this comment.
Oh, I now see why you checked for the token kind.
Let me give you some compiler background: Compilers are usually divided in multiple stages. The interesting stages here are lexing, parsing, name lookup and type checking.
The purpose of lexing is to take the contents of a file and form tokens from it. For example, it knows that two words separated by a space are two different identifiers instead of one. It also takes care of classifying comments. These are the files in SwiftParser/Lexer
Parsing than takes this stream of tokens and forms a tree from them. And if it can't form a tree from the tokens, the parser will throw an error. Parsing is still what we call syntactical, that is, it doesn't have a clue about types or which declarations a variable, function, etc. references. This is the part of the code base you are working on.
The next step is name lookup. Name lookup determines the declarations a variable/function/… reference might refer to. This stage can, for example, already take into account when one variable shadows another. Name lookup still doesn't concern itself with type information. If a function is overloaded, for example, it just returns an overload set of functions that might be applicable.
And after that there's type checking, which takes the structure from parsing and the information from type checking to check that all the types match. Because of Swift's rules that you can overload functions based on their return types and other type inference features, this is a whole lot more complicated beast.
Having said that, name lookup and type checking are not implemented in the swift-syntax repo yet, so there is no way you can check that the types of the arguments match.
There was a problem hiding this comment.
Ah, I see! Now it all makes sense.
I sincerely appreciate your detailed explanation.
Thanks to you, I was able to understand it easily 🙇♂️
TTOzzi
force-pushed
the
fix-diagnostic-of-ternary-operator-missing-only-colon
branch
from
c7a1442 to
f302ac9
Compare
| @@ -317,16 +317,7 @@ extension Parser { | |||
| arena: self.arena | |||
| ) | |||
|
|
|||
| let rhs: RawExprSyntax? | |||
| if colon.isMissing { | |||
There was a problem hiding this comment.
Contrary to my first comment, I just had a thought. What do we produce if we have the following?
condition ? 1
someOtherVariable
I think in this case we should be suggesting to add : <#expression#> and not just a standalone colon. To achieve that you might need to revert to the old code and just add an additional check for whether the next token is on the same line or a new line.
There was a problem hiding this comment.
In the latest commit of this PR, it returns the diagnostic "expected ':' after '? ...' in ternary expression."
The Swift compiler in Xcode works the same way.
However, as you mentioned, I think it is appropriate to suggest adding : <#expression#> to the code.
I will proceed to work on it further and add a commit in this PR!
|
@swift-ci Please test |
Resolve #1624
Previously, in the ternary operator, the absence of a colon always resulted in parsing it as the right-hand side expression being missing.
Added logic to check if the next token is the same kind as the token before the colon when parsing a ternary operator, if there is no colon.