Skip to content

Correctly set IsRetargetable for source assemblies#55066

Merged
jcouv merged 2 commits intodotnet:mainfrom
333fred:sourceassemblysymbol-retargetable
Jul 24, 2021
Merged

Correctly set IsRetargetable for source assemblies#55066
jcouv merged 2 commits intodotnet:mainfrom
333fred:sourceassemblysymbol-retargetable

Conversation

@333fred
Copy link
Copy Markdown
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
@ghost ghost added the Area-Compilers label Jul 23, 2021
@333fred
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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)

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

jcouv commented Jul 23, 2021

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

@333fred
Copy link
Copy Markdown
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
Copy Markdown
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