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

[hot reload] Mono needs to be more lax about CustomAttribute updates #57387

Closed
lambdageek opened this issue Aug 14, 2021 · 1 comment · Fixed by #57388
Closed

[hot reload] Mono needs to be more lax about CustomAttribute updates #57387

lambdageek opened this issue Aug 14, 2021 · 1 comment · Fixed by #57388
Assignees
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Aug 14, 2021

The runtime accepts updates to a CustomAttribute row, as long as the parent and the "type" (really ctor) are "the same".

But the check for whether the ctor is "the same" is too strict - it expects token equality. That works for MethodDef tokens, but not for MemberRef tokens, because Roslyn emits fresh MemberRef (and TypeRef and AssemblyRef) rows into the delta.
So the token for the new ctor is not literally the same as the token for the old ctor.

We should trust that Roslyn won't send us anything unexpected - which it does not because changing the ctor or its argument data requires the ChangeCustomAttributes capability, which mono doesn't provide).

This particularly affects this-capturing lambdas.

Something like this:

  string field;
  void Method() {
    Action f = () => field += ".";
    f();
  }

Generates

   private string field;

    public void Method() { new Action(<Method>b__1_0)(); }

    [CompilerGenerated] // <------  this is a MemberRef
    private void <Method>b__1_0() { field += "."; }
@lambdageek lambdageek added the area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc label Aug 14, 2021
@lambdageek lambdageek added this to the 6.0.0 milestone Aug 14, 2021
@lambdageek lambdageek self-assigned this Aug 14, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 14, 2021
@lambdageek
Copy link
Member Author

Happens in Razor pages with a @bind because that generates a this-capturing lambda dotnet/aspnetcore#34446

@lambdageek lambdageek removed the untriaged New issue has not been triaged by the area owner label Aug 14, 2021
lambdageek added a commit to lambdageek/runtime that referenced this issue Aug 14, 2021
…tes.

If the "type" (really .ctor) token is a MemberRef, it won't be literally the
same token value in the update as in the baseline because Roslyn emits
additional MemberRef, TypeRef and AssemblyRef rows instead of trying to reuse
the baseline ones.

We could try to resolve the baseline and delta tokens and check that they map
to the same MonoMethod*, but that is problematic (it may trigger assembly
loading and class initialization) and also unnecessary - without the
ChangeCustomAttribute capability Roslyn will not allow edits that change the
constructor or its arguments.  So just drop the extra check.

Fixes dotnet#57387
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 14, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 14, 2021
lambdageek added a commit that referenced this issue Aug 14, 2021
…tes (#57388)

* [hot_reload] Allow the .ctor token to change for CustomAttribute updates.

If the "type" (really .ctor) token is a MemberRef, it won't be literally the
same token value in the update as in the baseline because Roslyn emits
additional MemberRef, TypeRef and AssemblyRef rows instead of trying to reuse
the baseline ones.

We could try to resolve the baseline and delta tokens and check that they map
to the same MonoMethod*, but that is problematic (it may trigger assembly
loading and class initialization) and also unnecessary - without the
ChangeCustomAttribute capability Roslyn will not allow edits that change the
constructor or its arguments.  So just drop the extra check.

Fixes #57387

* Add testcase
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant