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

Mark Assembly.CodeBase / Assembly.EscapedCodeBase as obsolete (#31127) #39261

Merged

Conversation

JoshSchreuder
Copy link
Contributor

No description provided.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@JoshSchreuder JoshSchreuder force-pushed the feature/assembly-codebase-obsolete branch from 16d8a3f to a0562f3 Compare July 14, 2020 07:58
@JoshSchreuder JoshSchreuder force-pushed the feature/assembly-codebase-obsolete branch from a0562f3 to 34c156d Compare July 17, 2020 10:20
@JoshSchreuder JoshSchreuder force-pushed the feature/assembly-codebase-obsolete branch 4 times, most recently from 67d8bdf to 9f63055 Compare July 19, 2020 05:11
@JoshSchreuder JoshSchreuder force-pushed the feature/assembly-codebase-obsolete branch from 1af9f58 to b560587 Compare July 19, 2020 07:11
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

One tweak needed in a ref file; otherwise the obsoletion looks good. I am not familiar with the functional changes being made though, so we need to get another reviewer for those changes.

Thanks!

@@ -9,6 +9,7 @@ namespace System.Reflection.Emit
public sealed partial class AssemblyBuilder : System.Reflection.Assembly
{
internal AssemblyBuilder() { }
[Obsolete("CodeBase and EscapedCodeBase are only included for .NET Framework compatibility. Use Location instead.", DiagnosticId = "SYSLIB0012", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
Copy link
Member

Choose a reason for hiding this comment

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

Ref sources also need to fully qualify System.ObsoleteAttribute.

Copy link
Contributor Author

@JoshSchreuder JoshSchreuder Jul 22, 2020

Choose a reason for hiding this comment

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

@jeffhandley your one from #39269 didn't have this which I copied 😄

329f0db#diff-ef696a2876ba83b0d62cc5506022975c

Do you want me to update both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated both of these to use System.ObsoleteAttribute

// Assemblies loaded in memory return empty string from Location.
// The code below throws an exception if this happens.
// Catching exceptions here is not a bad thing.
if (asm.Location == string.Empty)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to cache asm.Location in a local to avoid calling it twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated

Comment on lines 81 to 82
// The code below throws an exception if this happens.
// Catching exceptions here is not a bad thing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The code below throws an exception if this happens.
// Catching exceptions here is not a bad thing.

These comments do not look helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed comment

Comment on lines 103 to 105
// Location is not supported by emitted assemblies and this will throw an exception
// down below.
else if (!resourceAssembly.IsDynamic && resourceAssembly.Location != string.Empty)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Location is not supported by emitted assemblies and this will throw an exception
// down below.
else if (!resourceAssembly.IsDynamic && resourceAssembly.Location != string.Empty)
else if (!resourceAssembly.IsDynamic)

I do not think it is necessary to check for empty location up-front here.

Copy link
Contributor Author

@JoshSchreuder JoshSchreuder Jul 22, 2020

Choose a reason for hiding this comment

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

@jkotas do you think it should be something more like

else if (!resourceAssembly.IsDynamic)
{
    // Emitted assemblies return empty string from Location.
    var location = asm.Location;
    if (location != string.Empty) 
    {
        string fileName = new FileInfo(location).Name;
        ...
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be:

else
{
    string location = asm.Location;
    if (location != string.Empty)
    {
        string fileName = Path.GetFileName(location);
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated as suggested, thanks!

@JoshSchreuder JoshSchreuder force-pushed the feature/assembly-codebase-obsolete branch from 0f514c4 to 43a8c26 Compare July 26, 2020 01:42
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit b19d798 into dotnet:master Jul 26, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants