Skip to content

Ensure declaring compilation can be located for source symbols created during compilation for EE evaluation.#60152

Merged
AlekseyTs merged 1 commit intodotnet:mainfrom
AlekseyTs:Issue59093
Mar 16, 2022
Merged

Ensure declaring compilation can be located for source symbols created during compilation for EE evaluation.#60152
AlekseyTs merged 1 commit intodotnet:mainfrom
AlekseyTs:Issue59093

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

Fixes #59093.
Related to #58198.

…d during compilation for EE evaluation.

Fixes dotnet#59093.
Related to dotnet#58198.
@AlekseyTs AlekseyTs requested review from cston and jcouv March 13, 2022 19:50
@AlekseyTs AlekseyTs requested a review from a team as a code owner March 13, 2022 19:50
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

@jcouv, @cston, @dotnet/roslyn-compiler Please review.

(object)null);
_allTypeParameters = container.TypeParameters.Concat(_typeParameters);
this.TypeMap = new TypeMap(allSourceTypeParameters, _allTypeParameters);
_allTypeParameters = container.TypeParameters.Concat(_typeParameters).Concat(_typeParameters);
Copy link
Copy Markdown
Member

@chsienki chsienki Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the double Concat? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of the double Concat?

We added some new type parameters to allSourceTypeParameters above, they should be pared with entries in _allTypeParameters. They should be paired with the same _typeParameters.

@jcouv jcouv self-assigned this Mar 14, 2022
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Mar 15, 2022

@jcouv, @cston, @dotnet/roslyn-compiler Please review.

1 similar comment
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented Mar 15, 2022

@jcouv, @cston, @dotnet/roslyn-compiler Please review.

{
if (IsViableSourceMethod(candidateMethod, desiredMethodName, desiredTypeParameters, sourceMethodMustBeInstance))
{
MethodSymbol sourceMethod = new EECompilationContextMethod(candidateSubstitutedSourceMethod.DeclaringCompilation!, candidateMethod.OriginalDefinition);
Copy link
Copy Markdown
Contributor

@cston cston Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!

Is ! necessary? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ! necessary?

Yes, despite the assert above, a nullable warning is reported here. I will open an issue after the change is merged.

public PlaceholderLocal(Symbol containingSymbol, object identifier, TypeWithAnnotations type)
{
Debug.Assert(identifier != null);
Debug.Assert(containingSymbol is null || containingSymbol.DeclaringCompilation is not null);
Copy link
Copy Markdown
Contributor

@cston cston Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug.Assert(containingSymbol is null || containingSymbol.DeclaringCompilation is not null);

Could we also assert something similar in AbstractFlowPass constructor?

Debug.Assert(_symbol is null || _symbol.DeclaringCompilation is not null);
``` #WontFix

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there are other scenarios (not EE related) when this assert is going to fail. For whatever reason AttributeSemanticModel uses the attribute type as member symbol and it is used as the symbol for region flow analysis, which makes little sense in my opinion.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 1)

@AlekseyTs AlekseyTs merged commit 50e7f9d into dotnet:main Mar 16, 2022
@ghost ghost added this to the Next milestone Mar 16, 2022
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NullReferenceException in AbstractFlowPass evaluating local function with switch statement in EE

5 participants