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 as obsolete #31127

Closed
terrajobst opened this issue Oct 9, 2019 · 15 comments
Closed

Mark Assembly.CodeBase as obsolete #31127

terrajobst opened this issue Oct 9, 2019 · 15 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@terrajobst
Copy link
Member

Assembly.Location and Assembly.CodeBase are very similar, but not the same thing. To reduce confusion, we should mark it as obsolete.

Reason

Having two things that are similar, but not identical always causes confusion. Here is an example.

As @jkotas said:

Assembly.Location is strictly better than Assembly.CodeBase.

Assembly.CodeBase is an obsolete property. The only reason why it was included in .NET Core was .NET Framework compatibility. The original purpose of Assembly.CodeBase was CAS (Code Access Security). It was meant to describe where the assembly was downloaded from for the Internet Zone security checks. It also explains some of its weird behaviors. For example, if the assembly is loaded as byte array, it returns the location of the caller of the Assembly.Load method.

Proposed API

namespace System.Reflection
{
    public partial class Assembly
    {
        [Obsolete("Use Location instead.")]
        public virtual string CodeBase { get; }
    }
}
@terrajobst
Copy link
Member Author

terrajobst commented Oct 22, 2019

Video

Looks good. We should also mark it as [EditorBrowsable(Never)].

namespace System.Reflection
{
    public partial class Assembly
    {
        [Obsolete("Use Location instead.")]
        public virtual string CodeBase { get; }
    }
}

@vitek-karas
Copy link
Member

Should we also consider obsoleting AssemblyName.CodeBase? It's not exactly the same thing, but quite related - in fact that property has no functional effect (trying to load assembly via AssemblyName will ignore the CodeBase property anyway: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/Assembly.CoreCLR.cs#L63

@jkotas
Copy link
Member

jkotas commented Jan 30, 2020

Assembly.EscapedCodeBase should be marked as obsolete as well.

@bitbonk
Copy link
Contributor

bitbonk commented Jan 30, 2020

Just out of curiosity: Since those APIs are part of .NET Standard, what implications does marking APIs as obsolete have for .NET Standard?

Since in .NET Standard, APIs cannot be removed, only added, does that mean you will only obsolete those APs but never remove them? Or will .NET Standard itself become obsolete due to the unification in .NET 5.

@jkotas
Copy link
Member

jkotas commented Jan 30, 2020

Right, we do not have plans to remove any netstandard APIs.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@steveharter steveharter added good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Jul 7, 2020
@JoshSchreuder
Copy link
Contributor

JoshSchreuder commented Jul 14, 2020

I started looking at this, but need a little clarification as I'm unfamiliar with the code structure.

With my change, I'm getting the following errors where CodeBase / EscapedCodeBase are used elsewhere (DesigntimeLicenseContext, Helper.Win32Unix, RoAssembly). Can I please get some tips on the best way to resolve these?

System\ComponentModel\Design\DesigntimeLicenseContext.cs(88,56): error CS0618: 'Assembly.EscapedCodeBase' is obsolete: 'EscapedCodeBase is only included for .NET Framework compatibility. Use Location instead.' [C:\Work\dotnetruntime\src\libraries\System.ComponentModel.TypeConverter\src\System.ComponentModel.TypeConverter.csproj]
System\ComponentModel\Design\DesigntimeLicenseContext.cs(111,45): error CS0618: 'Assembly.EscapedCodeBase' is obsolete: 'EscapedCodeBase is only included for .NET Framework compatibility. Use Location instead.' [C:\Work\dotnetruntime\src\libraries\System.ComponentModel.TypeConverter\src\System.ComponentModel.TypeConverter.csproj]
System\IO\IsolatedStorage\Helper.Win32Unix.cs(57,36): error CS0618: 'Assembly.CodeBase' is obsolete: 'CodeBase is only included for .NET Framework compatibility. Use Location instead.' [C:\Work\dotnetruntime\src\libraries\System.IO.IsolatedStorage\src\System.IO.IsolatedStorage.csproj]
System\Reflection\TypeLoading\Assemblies\RoAssembly.cs(44,39): error CS0672: Member 'RoAssembly.CodeBase' overrides obsolete member 'Assembly.CodeBase'. Add the Obsolete attribute to 'RoAssembly.CodeBase'. [C:\Work\dotnetruntime\src\libraries\System.Reflection.MetadataLoadContext\src\System.Reflection.MetadataLoadContext.csproj]
System\Reflection\TypeLoading\Assemblies\RoAssembly.cs(45,39): error CS0672: Member 'RoAssembly.EscapedCodeBase' overrides obsolete member 'Assembly.EscapedCodeBase'. Add the Obsolete attribute to 'RoAssembly.EscapedCodeBase'. [C:\Work\dotnetruntime\src\libraries\System.Reflection.MetadataLoadContext\src\System.Reflection.MetadataLoadContext.csproj]

Additionally, does this need something done per Better Obsoletions? How would I go about getting an aka.ms to put into list-of-obsoletions.md?

@jkotas
Copy link
Member

jkotas commented Jul 14, 2020

System.ComponentModel.TypeConverter should be switched to use Assembly.Location. The rest should use #pragmas to disable the warnings.

does this need something done per Better Obsoletions?

@jeffhandley is working on the plan for better obsoletions IDs. You can leave the ID as TODO in the PR.

@JoshSchreuder
Copy link
Contributor

Cheers @jkotas, I've raised #39261

@jkotas
Copy link
Member

jkotas commented Jul 17, 2020

We should also mark it as [EditorBrowsable(Never)]

@terrajobst Is marking it as [EditorBrowsable(Never)] still valid with the new obsoletion plan? We are marking a lot of other methods obsolete, but we are not marking them as [EditorBrowsable(Never)]. What is special about this method that it deserves to be marked with [EditorBrowsable(Never)]?

cc @jeffhandley

@jeffhandley
Copy link
Member

Per the non-goals of the better obsoletion spec, it is not expected that we'll mark [EditorBrowsable(Never)] in conjunction with the obsoletion but that they should be independent decisions. I took the approach to not apply the EditorBrowsable attribute in the big obsoletion PR.

I recommend we remove the EditorBrowable attributes from the obsoletions here too.

@JoshSchreuder
Copy link
Contributor

Updated PR with the new Obsoletion stuff, and removed the EditorBrowsable attribute, thanks all!

@vitek-karas
Copy link
Member

Should we also look into AssemblyName.CodeBase and AssemblyName.EscapedCodeBase - they are very similar to Assembly.CodeBase:

  • Runtime fills in the same values for loaded assemblies
  • When constructed manually the value is preserved
  • Runtime never actually consumes these values - calling Assembly.Load(AssemblyName) completely ignores these properties on the name.

@danmoseley
Copy link
Member

Was there an answer to this question?

Should we also look into AssemblyName.CodeBase and AssemblyName.EscapedCodeBase - they are very similar to Assembly.CodeBase:

@terrajobst
Copy link
Member Author

Yes, @jkotas mentioned it above:

Assembly.EscapedCodeBase should be marked as obsolete as well.

And the PR marked it.

@vitek-karas
Copy link
Member

What about the AssemblyName variants - they return basically the exact same thing... and AFAIK are not marked right now.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Reflection good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

10 participants