Improve and clarify the logic when speculatively comparing old/new code to ensure semantics are the same. #69261
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| semanticModel, | ||
| cancellationToken, | ||
| skipVerificationForReplacedNode: true, | ||
| skipVerificationForReplacedNode: false, |
There was a problem hiding this comment.
changed this (was a hack originally). we were previously making a change, but then skipping checking the actual node we were replacing. now we check it like all the rest above it to make sure it's ok.
| @@ -722,28 +722,65 @@ private bool ReplacementBreaksQueryClause(QueryClauseSyntax originalClause, Quer | |||
| !SymbolInfosAreCompatible(originalClauseInfo.OperationInfo, newClauseInfo.OperationInfo); | |||
| } | |||
|
|
|||
| protected override bool ReplacementIntroducesErrorType(ExpressionSyntax originalExpression, ExpressionSyntax newExpression) | |||
| protected override bool ReplacementIntroducesDisallowedNullType( | |||
There was a problem hiding this comment.
broke this into two checks. a check for error-types (always bad), and a check for a null type (sometimes bad, sometimes ok).
|
|
||
| // If we didn't have an error before, but now we got one, that's bad and should block conversion in all cases. | ||
| if (newTypeInfo.Type.IsErrorType() && !originalTypeInfo.Type.IsErrorType()) | ||
| return true; |
There was a problem hiding this comment.
this is inlining the error check from before. it is not something that should be overridable.
| if (newTypeInfo.Type.IsErrorType() && !originalTypeInfo.Type.IsErrorType()) | ||
| return true; | ||
|
|
||
| if (ReplacementIntroducesDisallowedNullType(originalExpression, newExpression, originalTypeInfo, newTypeInfo)) |
There was a problem hiding this comment.
it is then followed by the null-type check. where the language can override if null is a problem or not.
| semanticModel, | ||
| cancellationToken, | ||
| skipVerificationForReplacedNode: true, |
There was a problem hiding this comment.
changed this (was a hack originally). we were previously making a change, but then skipping checking the actual node we were replacing. now we check it like all the rest above it to make sure it's ok.
| SyntaxKind.ThisConstructorInitializer or | ||
| SyntaxKind.BaseConstructorInitializer or | ||
| SyntaxKind.EqualsValueClause or | ||
| SyntaxKind.ArrowExpressionClause; |
There was a problem hiding this comment.
just a simplification.
| @@ -506,7 +498,7 @@ protected override bool ReplacementChangesSemanticsForNodeLanguageSpecific(Synta | |||
| } | |||
| else if (currentOriginalNode.Kind() == SyntaxKind.ImplicitArrayCreationExpression) | |||
| { | |||
| return !TypesAreCompatible((ImplicitArrayCreationExpressionSyntax)currentOriginalNode, (ImplicitArrayCreationExpressionSyntax)currentReplacedNode); | |||
| return !TypesAreCompatible((ExpressionSyntax)currentOriginalNode, (ExpressionSyntax)currentReplacedNode); | |||
There was a problem hiding this comment.
incorrect logic before. the point of this analyzer is that the old construct and new construct might not match, but semantics shoudl still be preserved. so, in this case, the new construct just needs to be an expression, not an implicit array like before.
| @@ -42,10 +42,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Utilities | |||
| ''' True if semantic analysis should fail when any of the invocation expression ancestors of <paramref name="expression"/> in original code has overload resolution failures. | |||
| ''' </param> | |||
| Public Sub New(expression As ExpressionSyntax, newExpression As ExpressionSyntax, semanticModel As SemanticModel, cancellationToken As CancellationToken, Optional skipVerificationForReplacedNode As Boolean = False, Optional failOnOverloadResolutionFailuresInOriginalCode As Boolean = False) | |||
| MyBase.New(expression, newExpression, semanticModel, cancellationToken, skipVerificationForReplacedNode, failOnOverloadResolutionFailuresInOriginalCode) | |||
| MyBase.New(expression, newExpression, semanticModel, skipVerificationForReplacedNode, failOnOverloadResolutionFailuresInOriginalCode, cancellationToken) | |||
There was a problem hiding this comment.
VB still has skipVerificationForReplacedNode in a few places. I don't love it and i'd like to remove in teh future as well. but out of scope for this PR.
...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AbstractSpeculationAnalyzer.cs
Show resolved
Hide resolved
...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AbstractSpeculationAnalyzer.cs
Show resolved
Hide resolved
…ities/AbstractSpeculationAnalyzer.cs
This reverts commit 1ea48da.
…abadi/roslyn into speculativeConversions
ghost
added this to the 
I'm not a fan of the existing style, which i feel was a little too brittle/unclear. The new style attempts to be much crisper about what it is doing and why.