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

Add RequiresAssemblyFiles attribute #56196

Merged
merged 16 commits into from
Jul 30, 2021
Merged

Conversation

agocke
Copy link
Member

@agocke agocke commented Jul 23, 2021

This PR annotates every API doc'd as having different
behavior in single-file publishing with RequiresAssemblyFiles.
Some APIs might be special-cased by the analyzer to produce
special messages, but RAF is useful for visual inspection
of the APIs. Module.Name and .FullyQualifiedName were also
unannotated.

Docs: https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file

Assembly.Location | Returns an empty string.
Module.FullyQualifiedName | Returns a string with the value of <Unknown> or throws an exception.
Module.Name | Returns a string with the value of <Unknown>.
Assembly.GetFile | Throws IOException.
Assembly.GetFiles | Throws IOException.
Assembly.CodeBase | Throws PlatformNotSupportedException.
Assembly.EscapedCodeBase | Throws PlatformNotSupportedException.
AssemblyName.CodeBase | Returns null.
AssemblyName.EscapedCodeBase | Returns null.

This PR annotates every API doc'd as having different
behavior in single-file publishing with RequiresAssemblyFiles.
Some APIs might be special-cased by the analyzer to produce
special messages, but RAF is useful for visual inspection
of the APIs. Module.Name and .FullyQualifiedName were also
unannotated.
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@jeffhandley
Copy link
Member

@buyaa-n This PR is assigned to you for follow-up before the RC1 snap.

@agocke
Copy link
Member Author

agocke commented Jul 27, 2021

@buyaa-n anything else?

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Left two minor comments, overall LGTM

@agocke
Copy link
Member Author

agocke commented Jul 28, 2021

@buyaa-n If you wouldn't mind taking another look, I found some more missing annotations, and an inconsistency in casing that I've tried to fix up.

@VSadov @vitek-karas for change in casing in runtime. In theory this could be a breaking change for people upgrading from .NET 5 to .NET 6 but I think the inconsistent casing meant it was impossible for them to rely on specific behavior in this situation anyway. I'd rather settle on one casing for .NET 6.

@jkotas
Copy link
Member

jkotas commented Jul 28, 2021

for change in casing in runtime. In theory this could be a breaking change for people upgrading from .NET 5 to .NET 6 but I think the inconsistent casing meant it was impossible for them to rely on specific behavior in this situation anyway.

I believe that the runtime reflection APIs have been consistently returning uppercase <Unknown> for images loaded as byte arrays since forever. I am not sure why you think it was impossible for apps and libraries to depend on this specific behavior.

@agocke
Copy link
Member Author

agocke commented Jul 29, 2021

OK, I think this is finally up-to-date. I ended up finding a questionable behavior in single-file for Module.ScopeName which needs to be investigated separately. I filed #56519 to track that. I'd rather address that separately than block the rest of this PR on that small issue.

@agocke
Copy link
Member Author

agocke commented Jul 30, 2021

@buyaa-n Anything else? I think the one tricky bit I've filed an issue for, so we can come back to it later.

I took Jan's advice and changed the casing of <unknown> to <Unknown> for MetadataLoadContext, so it now matches the years-old behavior of the runtime.

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

One NIT comment, overall LGTM

@agocke agocke merged commit 4500583 into dotnet:main Jul 30, 2021
@agocke agocke deleted the finish-annotating-raf branch July 30, 2021 20:21
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants