Qualify the type when negating a pattern to a not pattern #72466
Conversation
...s/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/SyntaxGeneratorExtensions_Negate.cs
Outdated
Show resolved
Hide resolved
| syntaxFacts.SupportsNotPattern(semanticModel.SyntaxTree.Options)) | ||
| { | ||
| var typeNode = generatorInternal.Type(isTypeOperation.TypeOperand, false); | ||
| return generatorInternal.IsPatternExpression(leftOperand, operatorToken, generatorInternal.NotPattern(generatorInternal.ConstantPattern(typeNode))); |
There was a problem hiding this comment.
should this be a TypePattern instead of a ConstantPattern? Or is it actually supposed to be a ConstantPattern from teh perspective of the parser?
There was a problem hiding this comment.
i don't really see what the IIsTypeOperation { TypeOperand.SpecialType: SpecialType.None } isTypeOperation accomplishes. What if someone has an Int32 Int32 = 0 line and does !(o is Int32). Won't we have the same problem?
There was a problem hiding this comment.
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.
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.
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.
should this be a TypePattern instead of a ConstantPattern?
If the tests for the affected code fixes use the new testing library, it will validate that the nodes produced by the code fix are the same as the nodes produced by the compiler during a parsing operation on the same text.
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.
@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.
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.
I have more prs than usual pending other than this one (some haven't gotten a review yet)
Please ping on prs you'd like eyes on. Thanks.
There was a problem hiding this comment.
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.
@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:
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.
| // 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) && |
There was a problem hiding this comment.
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.
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.
can i make edits to your PR?
|
@sdelarosbil Done :) |
|
@CyrusNajmabadi I ran the test that was failing locally before pulling your changes and it passed, but this is definitely a failure caused by the pr. So it seems your changes broke the test. |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
Thanks! |
Closes #72370
When we run into a case like in this issue, we want to qualify the type name such that it resolves the ambiguity and let the simplifier reduce it to the shortest available.
However, simply using the full type expression wasn't enough: it turns out that a not pattern assembled this way requires the member expression version of the node instead of the qualified name one and that the pattern is a constant one instead of a type one. This is because unless it's a predefined type, it's seen as an expression rather than a type name.
With both of these changes, it's enough to resolve the ambiguities.