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

Correctly set IsRetargetable for source assemblies #55066

Merged
merged 2 commits into from
Jul 24, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jul 23, 2021

Source assemblies weren't checking assembly flags and setting IsRetargetable, despite having already decoded all well-known attributes to find the other bits of the assembly identity. We now check and correctly set the IsRetargetable bit. Fixes #54836.

Source assemblies weren't checking assembly flags and setting IsRetargetable, despite having already decoded all well-known attributes to find the other bits of the assembly identity. We now check and correctly set the IsRetargetable bit. Fixes dotnet#54836.
@333fred 333fred requested a review from a team as a code owner July 23, 2021 00:55
@333fred
Copy link
Member Author

333fred commented Jul 23, 2021

@dotnet/roslyn-compiler for review.

[assembly: AssemblyFlags(AssemblyNameFlags.Retargetable)]";

var comp = CreateCompilation(code);
Assert.True(comp.Assembly.Identity.IsRetargetable);
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the main purpose of AssemblyNameFlags.Retargetable is to cause a flag to be emitted in metadata.
FWIW, it looks like that was working correctly despite this bug in AssemblyIdentity (see test AssemblyFlagsAttribute which checks the emitted metadata).

Copy link
Member Author

Choose a reason for hiding this comment

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

AssemblyFlags is just written out directly, it doesn't read from Identity at all when doing so: https://sourceroslyn.io/#Microsoft.CodeAnalysis/PEWriter/MetadataWriter.cs,1969.

Copy link
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)

@jcouv jcouv self-assigned this Jul 23, 2021
@jcouv
Copy link
Member

jcouv commented Jul 23, 2021

Looks like there is a legitimate test failure: Microsoft.CodeAnalysis.UnitTests.AssemblyIdentityTests.InvalidConstructorArgs

@333fred
Copy link
Member Author

333fred commented Jul 23, 2021

Looks like there is a legitimate test failure: Microsoft.CodeAnalysis.UnitTests.AssemblyIdentityTests.InvalidConstructorArgs

Yeah, it's calling the wrong constructor now. I'll need to modify the test in the morning.

@333fred
Copy link
Member Author

333fred commented Jul 23, 2021

@dotnet/roslyn-compiler for a second review.

@jcouv jcouv merged commit c58fbd5 into dotnet:main Jul 24, 2021
@ghost ghost added this to the Next milestone Jul 24, 2021
@333fred 333fred deleted the sourceassemblysymbol-retargetable branch July 24, 2021 00:49
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
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.

CSharpCompilation.Assembly doesn't honor retargetable assembly flag
4 participants