-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Qualify the type when negating a pattern to a not pattern #72466
Changes from 3 commits
e0ac997
11c1f9d
9e2a7f0
a1efd6a
212e461
be90730
f6667af
50d4ad5
af54a87
35d371c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -158,6 +158,23 @@ private static SyntaxNode GetNegationOfBinaryExpression( | |||
return generatorInternal.IsPatternExpression(leftOperand, operatorToken, generatorInternal.ConstantPattern(generator.NullLiteralExpression())); | ||||
} | ||||
|
||||
// With a not pattern, it is possible to have an ambiguity between a type name or a local identifier. | ||||
// For example, if a class is named C and we have: | ||||
// C C = new(); | ||||
// object O = C; | ||||
// if (O is not C) | ||||
// ... | ||||
// Then there is an ambiguity between the type C and the local named C. | ||||
// To address this, we use an expression with the fully qualified type name which the simplifier | ||||
// will shorten to the shortest possible without ambiguity | ||||
if (!syntaxFacts.IsPredefinedType(rightOperand) && | ||||
operation is IIsTypeOperation isTypeOperation && | ||||
syntaxFacts.SupportsNotPattern(semanticModel.SyntaxTree.Options)) | ||||
{ | ||||
var typeNode = generatorInternal.Type(isTypeOperation.TypeOperand, typeContext: false); | ||||
return generatorInternal.IsPatternExpression(leftOperand, operatorToken, generatorInternal.NotPattern(generatorInternal.ConstantPattern(typeNode))); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't really see what the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I originally tried TypePattern and it turns out this is not only what cuased the simplifier issue I mentioned in the issue thread, but I found out later that it actually messes up the expected syntax tree. It means that yes, constant pattern is what's supposed to be there from the parser's perspective. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. So why not always make a constant pattern. Or make it a constant pattern if it's a NameSyntax? The IIs Check seems busted to me since we're making syntax checks, but trying to use semantics to drive it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to make it always a constant pattern and it wouldn't work because you get the same problem where if the right operand is "string", it messes up the syntax tree where it expected a type pattern when parsing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, this is why I ran into tests issues and I came to that same conclusion meaning I was somehow messing up the tree and changing the arrangement of nodes is how I figured out hwo to not have that happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CyrusNajmabadi I am still waiting an answer from you on this review. Also sidenote, I have more prs than usual pending other than this one (some haven't gotten a review yet) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should make the correct syntax based on what you are generating. If you are generating a predefined type syntax, then make the right sort of pattern. Same for the other type syntaxes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please ping on prs you'd like eyes on. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi ok I changed the checks to rely more on SyntaxFacts, but there is one confusion that I am not sure is what you wanted so I'll point it out. I absolutely need to know the TypeOperand of the IIstypeOperation here to generate to appropriate Type syntax.. I didn't see any api that does the same, but on the syntax node instead. In fact, checking this comment tells more or less why the syntax changes depending on this: Line 76 in 991d1f2
In other words, I don't know how I can avoid doing a semantic check in any way because I need the actual type symbol to generate the syntax and to do this, as far as I can see, I need to cast the IOperation to IIsTypeOperation. By this codepath, we already confirmed syntaxically that this is an IsTypeExpression so it seems safe to me, but I just want to make sure here because it seems to be the part you had a problem with. |
||||
} | ||||
|
||||
// `is y` -> `is not y` | ||||
if (syntaxFacts.SupportsNotPattern(semanticModel.SyntaxTree.Options)) | ||||
return generatorInternal.IsPatternExpression(leftOperand, operatorToken, generatorInternal.NotPattern(generatorInternal.TypePattern(rightOperand))); | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think a better fix is to get the typeNode back, and then determine what sort of pattern needs to be generated based on that typeNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I am confused on how one would do it here: the original expression at best gives me a PredefinedTypeSyntax or an IdentifierNameSyntax. That is the only information I have here to determine which pattern it should be and IsPredefinedType covers this check.
If it's not a predefined type, I have to generate a fully qualified name of the type and wrap it in a constant pattern. The typeNode is only involved there because this is when I know the format it should have.
I think I am missunderstanding because it sounds like you're asking to obtain something I can't get or know if I should get before checking what type rightOperand is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can i make edits to your PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, go ahead.