Skip to content
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

Adjust ContainingSymbol for substituted local function #75636

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Oct 25, 2024

Fixes #75543

@jcouv jcouv self-assigned this Oct 25, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 25, 2024
@jaredpar jaredpar added this to the 17.12 milestone Oct 28, 2024
@jcouv jcouv modified the milestones: 17.12, 17.13 Oct 30, 2024
@jcouv jcouv force-pushed the local-containing branch 3 times, most recently from 938f2ed to 6a27f97 Compare November 4, 2024 23:17
@jcouv jcouv marked this pull request as ready for review November 5, 2024 20:16
@jcouv jcouv requested a review from a team as a code owner November 5, 2024 20:16
@@ -190,7 +190,9 @@ public sealed override Symbol ContainingSymbol
{
get
{
return _containingType;
return MethodKind is MethodKind.LocalFunction
? OriginalDefinition.ContainingSymbol
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2024

Choose a reason for hiding this comment

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

OriginalDefinition.ContainingSymbol

This will be valid only when we are dealing with local function symbol with container that is not substituted itself. At the moment this assumption is probably true because we never create such a symbol for a local function, not that it is technically impossible. At the same time we have an invariant that a constructed generic symbol must maintain ContainingSymbol === ConstructedFrom.ContainingSymbol. It is not obvious that the conditional logic around local function is going to maintain the invariant under all possible scenarios now and in the fututre. Consider if it would be more intuitive and more maintainable in the long run to do the following instead:

  1. Override ContainingSymbol in ConstructedMethodSymbol as follows:
        public override Symbol ContainingSymbol
        {
            get
            {
                return ConstructedFrom.ContainingSymbol;
            }
        }
  1. Add the following assert to protected SubstitutedMethodSymbol(NamedTypeSymbol containingSymbol, TypeMap map, MethodSymbol originalDefinition, MethodSymbol constructedFrom):
Debug.Assert(this is ConstructedMethodSymbol || originalDefinition.MethodKind is not (MethodKind.LocalFunction or MethodKind.Lambda));
``` #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 5, 2024

Choose a reason for hiding this comment

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

Here is another even more general alternative approach:

  1. Replace NamedTypeSymbol _containingType with Symbol _containingSymbol
  2. Change signature of the second constructor to protected SubstitutedMethodSymbol(Symbol containingSymbol, TypeMap map, MethodSymbol originalDefinition, MethodSymbol constructedFrom)
  3. Adjust implementation as follows:
public sealed override Symbol ContainingSymbol
{
    get
    {
        return _containingSymbol;
    }
}

public override NamedTypeSymbol ContainingType
{
    get
    {
        return _containingSymbol is NamedTypeSymbol nt ? nt : containingSymbol.ContainingType;
    }
}
  1. Adjust constructor of ConstructedMethodSymbol to do the following:
    base(containingSymbol: constructedFrom.ContainingSymbol, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks much for the alternative proposals

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@jcouv jcouv enabled auto-merge (squash) November 7, 2024 01:26
@jcouv jcouv merged commit b0b8e0f into dotnet:main Nov 7, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 17.13, Next Nov 7, 2024
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ContainingSymbol for substituted local function seems incorrect
5 participants