Coder Social home page Coder Social logo

Comments (53)

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024 13

Why not just add a flag to Obsolete to specify this behavior? Instead of bool error, create new enum ObsoleteLevel { Warning, Error, BinaryCompatOnly }, then obsolete the Obsolete constructor which accepts bool error (see what I did there?) and add a new overload which accepts the new enum.

from csharplang.

DanFTRX avatar DanFTRX commented on June 9, 2024 7

Why not just add a flag to Obsolete to specify this behavior? Instead of bool error, create new enum ObsoleteLevel { Warning, Error, BinaryCompatOnly }, then obsolete the Obsolete constructor which accepts bool error (see what I did there?) and add a new overload which accepts the new enum.

This would also have the added benefit of pushing developers to check over all their existing Obsolete attributes.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024 5

The compiler already queries for Obsolete, so the comparison isn't between an attribute with a flag and an attribute without a flag, rather between a new flag on an existing attribute, vs a whole new attribute - I think it's easier to just add a flag to Obsolete.

More importantly though, logically a 3-state Obsolete flag makes sense because it describes the 3 levels of "obsoletion" of a member; whereas with a new attribute, we would now have more possibilities: Obsolete(false); Obsolete(false), BinaryCompatOnly; Obsolete(true); Obsolete(true), BinaryCompatOnly; BinaryCompatOnly - 5 possibilities; obviously 5 is more than 3.

from csharplang.

rickbrew avatar rickbrew commented on June 9, 2024 5

Sign me up. This would be really useful for me. Paint.NET's plugin system is almost 20 years old, and sometimes it's quite difficult to modernize things, or to get plugin authors to use the correct (new) systems, when all the old stuff is still littering the namespaces and Intellisense.

For example, I have two BGRA32 color/pixel structs: PaintDotNet.ColorBgra, which is the old/legacy one, and then there's PaintDotNet.Imaging.ColorBgra32. How is a plugin author supposed to know which one they should use? Sometimes it's obvious, because they're passed a parameter that specifies it, e.g. IBitmap<ColorBgra32>, or they're similarly required to return something that uses the new type. But otherwise there's no way, other than [Obsolete], to keep them away from the old crusty version of the struct.

Here's another example. Let's say you're writing a library of primitives for things like vectors, matrices, colors, etc. Sometimes these should be explicitly castable to other primitives, other times implicitly castable.

But sometimes you make a mistake and an explicit cast operator really should've been implicit, or vice versa. How do you fix that? You literally can't. You are forever stuck with your first decision or typo or copypasta error. Edit: This is because the compiler won't let you declare both (without trickery), and if both exist when using it then you'll get ambiguity errors (IIRC).

public readonly struct Matrix5x4
{
    private readonly Vector4 row0, row1, row2, row3, row4;
    ...
    public static implicit operator Matrix4x4(Matrix5x4 matrix) => ...; // OOPS ๐Ÿ˜ข
}

I mention this because I actually made this type of mistake recently ๐Ÿ˜ข I got lucky -- no plugins were using the cast operator, so I just changed it.

It'd be nice to have this capability:

    [BinaryCompatOnly]
    public static implicit operator Matrix4x4(Matrix5x4 matrix) => (Matrix4x4)matrix;

    public static explicit operator Matrix4x4(Matrix5x4 matrix) => ...

BTW, extension methods already have a pseudo [BinaryCompatOnly] ... just remove the this https://twitter.com/rickbrewPDN/status/1146810316887429120

from csharplang.

HaloFour avatar HaloFour commented on June 9, 2024 3

Would the compiler resolve the annotated member as a last resort, for the purposes of providing an error message? Or would the member be effectively completely invisible to the compilation process?

I think it does make sense to evolve the ObsoleteAttribute rather than creating a new one. My one concern would be existing compilers wouldn't know to look for these changes, but as long as setting those new properties or whatever also results in IsError returning true it's probably not an issue.

from csharplang.

teo-tsirpanis avatar teo-tsirpanis commented on June 9, 2024 3

@rickbrew it can be integrated with the project files like this:

MyProject.csproj

<PropertyGroup>
  <TypeForwards>forwards.txt</TypeForwards>
</PropertyGroup>

forwards.txt

MyNamespace.MyObsoleteType[MyObsoleteAssembly]
MyNamespace.MyObsoleteType2[MyObsoleteAssembly]

Or this:

<ItemGroup>
  <TypeForward Include="MyNamespace.MyObsoleteType[MyObsoleteAssembly]" />
  <TypeForward Include="MyNamespace.MyObsoleteType2[MyObsoleteAssembly]" />
</ItemGroup>

from csharplang.

HaloFour avatar HaloFour commented on June 9, 2024 3

@TonyValenti

I'd like to be able to completely hide the method with no visibility and not even a discoverable name.

The purpose is to hide the method from the compiler, but not from the runtime. If the method was hidden from the runtime it wouldn't work for the purposes of maintaining binary compatibility.

from csharplang.

333fred avatar 333fred commented on June 9, 2024 2

Why not just add a flag to Obsolete to specify this behavior?

My concern with this is that the BCL may want to do this in libraries that are still targeting netstandard2.0. It's certainly something we can discuss though.

from csharplang.

CyrusNajmabadi avatar CyrusNajmabadi commented on June 9, 2024 2

they mentioned the possibility of overloading by return type thanks to this feature, if I recall correctly.

We'd have to update this proposal then in taht case, or add a new proposal for that :)

I'm not opposed to it. It just doesn't seem to be part of what is being designed right here, right now. Perhaps for the future.

from csharplang.

jnm2 avatar jnm2 commented on June 9, 2024 1

My concern with this is that the BCL may want to do this in libraries that are still targeting netstandard2.0. It's certainly something we can discuss though.

@333fred Couldn't such libraries still define an internal System.ObsoleteAttribute and silence the warning that a compilation type is being preferred over a conflicting library type?

from csharplang.

333fred avatar 333fred commented on June 9, 2024 1

Mainly because there are tons already that is already compiler generated for all TFMs

As I previously stated, the compiler does not generate attributes we expect users to apply by hand, and this policy will not change here. There are lots of footguns for polyfilling these attributes on lower TFMs, and we do not support the scenario.

from csharplang.

Neme12 avatar Neme12 commented on June 9, 2024 1

Is it yet decided whether this will allow overloading by return type? If so, we need new reflection APIs (e.g. Type.GetMethod) that take a return type in addition to the parameter types, in order to be future proof and not get an AmbiguousMatchException if a new, preferred overload with a different return type is added in the future.

from csharplang.

colejohnson66 avatar colejohnson66 commented on June 9, 2024

So, the idea is, should I consume a DLL with obsolete members, that I can swap out a dynamically invoked DLL with a newer version, but I wouldnโ€™t be able to compile against the newer one? In other words, the idea is to allow types that are visible to the runtime but not the compiler?

from csharplang.

333fred avatar 333fred commented on June 9, 2024

In other words, the idea is to allow types that are visible to the runtime but not the compiler?

Precisely.

from csharplang.

iam3yal avatar iam3yal commented on June 9, 2024

@TahirAhmadov I'd imagine it's easier to query whether this attribute exists than to query whether a property on it exists and then question whether its value is true/false, imo it makes little sense but not only that, overall having an attribute is cheaper, more readable and discoverable.

from csharplang.

iam3yal avatar iam3yal commented on June 9, 2024

@TahirAhmadov You might be right, I'm a bit torn on this but for whatever reason they chose to go with an attribute so maybe they can share their clarification in a comment or update the OP. :)

from csharplang.

ds5678 avatar ds5678 commented on June 9, 2024

Since the main motivation for this is to resolve overload resolution problems, I propose this be resolved by changing ObsoleteAttribute instead.

Idea 1: Make Obsolescence Affect Overload Resolution

We define an "obsolete context" where a containing type or member is obsolete. In this context, overload resolution is treated the same as it is today.

Outside of an obsolete context, obsolete members are treated as a last resort for resolution, perhaps even after extension members with the same signature.

Idea 2: Add a Boolean Property to ObsoleteAttribute

partial class ObsoleteAttribute
{
    bool BinaryCompatOnly { get; set; }
}

or an enum like the other commenter suggested.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024

@333fred isn't it the same issue with either adding a new attribute or modifying an existing one? Please elaborate...

from csharplang.

333fred avatar 333fred commented on June 9, 2024

No, it is not the same. You could polyfill BinaryCompatAttribute.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024

Hmmm...

If we were to change how Obsolete(error=true) behaves, the outcome would be overload resolution would work differently and programs which previously didn't compile would now compile - is that acceptable?

How about modifying Obsolete for new .NET and polyfilling a new Obsolete under a different namespace for netstandard2.0? Either using #if or even with compiler magic - if System.ObsoleteAttribute is referenced, C# redirects to System.BackCompat.ObsoleteAttribute.

Or just do both - modify Obsolete in new .NET; add BackCompatOnly as polyfill for netstandard2.0; then compiler checks both, with eventual netstandard2.0 deprecation it stops being used naturally - again, with #if.

These options may sound too complicated, but it just feels so right to modify Obsolete, it would be a shame to add a new attribute just because of the older framework compatibility.

from csharplang.

333fred avatar 333fred commented on June 9, 2024

If we were to change how Obsolete(error=true) behaves, the outcome would be overload resolution would work differently and programs which previously didn't compile would now compile - is that acceptable?

Absolutely not, but I think you may have known that ๐Ÿ˜›.

How about modifying Obsolete for new .NET and polyfilling a new Obsolete under a different namespace for netstandard2.0? Either using #if or even with compiler magic - if System.ObsoleteAttribute is referenced, C# redirects to System.BackCompat.ObsoleteAttribute.

That doesn't seem preferrable to me. That seems way more complex.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024

@jnm2 I think it's possible to create a Nuget which just adds source to the project - not a referenced DLL, right? Then this "internal" Obsolete can be distributed without the need to copy and paste (which by itself is not a huge amount of effort, but still)

from csharplang.

teo-tsirpanis avatar teo-tsirpanis commented on June 9, 2024

Moving from #7707 (comment), one idea to support type-forwarding these hidden types is to add a command-line option for a line-delimited file with the assembly-qualified names of the types to forward.

The compiler will check that these types exist in the referenced assembly, and emit entries in the ExportedType table.

from csharplang.

333fred avatar 333fred commented on June 9, 2024

My concern with this is that the BCL may want to do this in libraries that are still targeting netstandard2.0. It's certainly something we can discuss though.

@333fred Couldn't such libraries still define an internal System.ObsoleteAttribute and silence the warning that a compilation type is being preferred over a conflicting library type?

I could be misremembering, but no, I don't believe they could, just like (iirc) you can't do that with the Obsolete constructor that takes an error code.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024
using System;

namespace ObsoleteTest
{
	internal class Program
	{
		static void Main(string[] args)
		{
			Console.WriteLine("Hello, World!");
		}
	}
	[Obsolete(ObsoleteLevel.BinaryCompatOnly)]
	class Foo
	{
	}
}

namespace System
{
	enum ObsoleteLevel { Warning, Error, BinaryCompatOnly };
	class ObsoleteAttribute : Attribute
	{
		public ObsoleteAttribute(ObsoleteLevel level = ObsoleteLevel.Warning) { }
	}
}

It causes this currently: 1>C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Current\Bin\Roslyn\Microsoft.CSharp.Core.targets(80,5): error MSB6006: "csc.exe" exited with code -2146232797.

IDE shows this in stacktrace: Unable to cast object of type 'System.Int32' to type 'System.String'.

Looks like this is not currently supported, but surely this can be made to work? The legacy netstandard2.0 is unchanged, but the compiler can be changed to accommodate this, right?

from csharplang.

rickbrew avatar rickbrew commented on June 9, 2024

one idea to support type-forwarding these hidden types is to add a command-line option for a line-delimited file with the assembly-qualified names of the types to forward.

Please no ... I don't think you can even add custom command-line args for the C# compiler in Visual Studio

from csharplang.

rickbrew avatar rickbrew commented on June 9, 2024

Okay that would work :)

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024

Maybe it's a stupid idea - but can we re-use Obsolete.Message and add a special text at the end which denotes "binary compat only"? In new .NET, we add a ctor which sets the message as needed - together with IsError=true; new C# sees that message and treats it as "BinaryCompatOnly"; old C# just continues erroring as usual - and user sees something like "--THIS IS BINARY COMPAT ONLY--" at the end of the message; .NET standard 2.0 just adds that blob manually - using some polyfilled type with a constant string; voila.

from csharplang.

ds5678 avatar ds5678 commented on June 9, 2024

My concern with this is that the BCL may want to do this in libraries that are still targeting netstandard2.0. It's certainly something we can discuss though.

@333fred Couldn't such libraries still define an internal System.ObsoleteAttribute and silence the warning that a compilation type is being preferred over a conflicting library type?

I could be misremembering, but no, I don't believe they could, just like (iirc) you can't do that with the Obsolete constructor that takes an error code.

I think we should make it that a class in our assembly hides all external classes with the same full name. (Those classes could still be accessed with extern alias, if necessary.)

from csharplang.

333fred avatar 333fred commented on June 9, 2024

@ds5678 I am unsure what you mean by that or how it applies here.

from csharplang.

ds5678 avatar ds5678 commented on June 9, 2024
using System;

namespace ObsoleteTest
{
	internal class Program
	{
		static void Main(string[] args)
		{
			Console.WriteLine("Hello, World!");
		}
	}
	[Obsolete(ObsoleteLevel.BinaryCompatOnly)]
	class Foo
	{
	}
}

namespace System
{
	enum ObsoleteLevel { Warning, Error, BinaryCompatOnly };
	class ObsoleteAttribute : Attribute
	{
		public ObsoleteAttribute(ObsoleteLevel level = ObsoleteLevel.Warning) { }
	}
}

It causes this currently: 1>C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Current\Bin\Roslyn\Microsoft.CSharp.Core.targets(80,5): error MSB6006: "csc.exe" exited with code -2146232797.

IDE shows this in stacktrace: Unable to cast object of type 'System.Int32' to type 'System.String'.

Looks like this is not currently supported, but surely this can be made to work? The legacy netstandard2.0 is unchanged, but the compiler can be changed to accommodate this, right?

@333fred if classes in an assembly hid external classes, then this code would work.

from csharplang.

TonyValenti avatar TonyValenti commented on June 9, 2024

I love this idea but I feel like the attribute should also be a special attribute that the CLR is aware of, similar to the unsafe assessors.

I'd like to be able to completely hide the method with no visibility and not even a discoverable name.

from csharplang.

TahirAhmadov avatar TahirAhmadov commented on June 9, 2024

Right, if you don't want the method to be seen at run time, you can just delete it.
If you're referring to reflection APIs not "finding" this method, that is already easy to achieve by writing an extension method on Type like public static MethodInfo? GetMethodWithoutBinaryCompatOnly(this Type This, string name) { ... } - the body calls GetMethod and checks for the attribute.
I don't think GetMethod etc. can be modified to ignore methods with this attribute because that would be a breaking change. But you can confirm this by asking for this in https://github.com/dotnet/runtime/

from csharplang.

KalleOlaviNiemitalo avatar KalleOlaviNiemitalo commented on June 9, 2024

Is this going to affect runtime lookups on the dynamic type? If not, the spec should explicitly say so. If yes, then it requires changing the "effectively archived" Microsoft.CSharp library.

from csharplang.

sharwell avatar sharwell commented on June 9, 2024

Would it be possible to make the compiler just omit items marked with [BinaryCompatOnly] from the reference assembly?

from csharplang.

tannergooding avatar tannergooding commented on June 9, 2024

Would it be possible to make the compiler just omit items marked with [BinaryCompatOnly] from the reference assembly?

I think it would be better to preserve them, so that diagnostics can be greatly improved and compilers that don't understand BinaryCompatOnly can still function.

There is an entire UX around removing such APIs that needs to be considered.

from csharplang.

CodeBlanch avatar CodeBlanch commented on June 9, 2024

How this works is a bit out of my pay grade but two things...

  1. As far as user feedback, I would love to have something like this! In OpenTelemetry .NET I have some APIs that only still exist for binary compatibility.

  2. If [BinaryCompatOnly] or [Obsolete(BinaryCompatOnly = true)] causes things to not be visible to the compiler, would that make testing those things difficult? I would still like test coverage over APIs which exist solely for binary compatibility. What if the mechanism was instead some way to modify internal visibility?

    [RuntimeVisible]
    internal class SomeApi {}

from csharplang.

333fred avatar 333fred commented on June 9, 2024

I have opened a new version of this specification: #7906. The original BinaryCompatOnly attribute is included in the alternatives section, but this new version is the version I intend to take to LDM.

from csharplang.

JeremyKuhne avatar JeremyKuhne commented on June 9, 2024

Here is a concrete issue where we would use this in WinForms: dotnet/winforms#10924

from csharplang.

AraHaan avatar AraHaan commented on June 9, 2024

Or do something where the compiler generates a new attribute in all compilations regardless of the TFM they target (and mark it with [CompilerGenerated]? Then name it BinaryCompatOnly Or better yet name it PreferredOverload(string) where in the string parameter you specify the preferred overload version of it that exactly matches the signature in this form: ListViewItemCollection.AddRange(IList) (Using Winforms ListViewItemCollection as an example here). And then it tells the compiler EXACTLY which overload to use when it tries to select the wrong one and sees that metadata attribute on it.

from csharplang.

miyu avatar miyu commented on June 9, 2024

Aside from the improvements to ObsoleteAttribute, I'd love to see a general-purpose OverloadResolutionPriority attribute discussed in this proposal. This is especially useful in handling combinatoric argument lists, e.g. for collections:

Buffer(length);
Buffer(length, flags);
Buffer(length, flags, value);
Buffer(length, value);

I would like Buffer(length, default) to invoke Buffer(length, value). This would have different semantics than Buffer(length) for my case (e.g. whether an unmanaged buffer is zeroed). This is significantly more ergonomic than requiring users to specify a zero-value explicitly... frequently, when I run into scenarios like this right now, I end up having to just delete one of the constructors or reorder the arg list, which can be really ugly.

from csharplang.

333fred avatar 333fred commented on June 9, 2024

@miyu this proposal has no ObsoleteAttribute changes. This is about overload resolution priority.

from csharplang.

AraHaan avatar AraHaan commented on June 9, 2024

I would prefer a new attribute be added that gets generated by the compiler itself regardless of TFM if the language version is set to preview or latest (after the current preview langversion is released as stable).

Why? Would allow even normal libraries that say for example multi-target but also need to make hints such as this to the compiler.

from csharplang.

333fred avatar 333fred commented on June 9, 2024

We never take that approach for attributes we expect users to apply directly, and I wouldn't expect that to change with this proposal.

from csharplang.

Neme12 avatar Neme12 commented on June 9, 2024

I wouldn't conflate this feature with the Obsolete attribute, they seem orthogonal.

from csharplang.

jnm2 avatar jnm2 commented on June 9, 2024

@AraHaan Like with other statically-analyzed attributes, libraries can declare their own copies of the attributes as internal with #if !NET9_0_OR_GREATER around them.

from csharplang.

AraHaan avatar AraHaan commented on June 9, 2024

@AraHaan Like with other statically-analyzed attributes, libraries can declare their own copies of the attributes as internal with #if !NET9_0_OR_GREATER around them.

Yeah, but I would rather it be compiler generated for all TFMs though. Mainly because there are tons already that is already compiler generated for all TFMs anyways nowadays so what is the problem with 1 more?

from csharplang.

HaloFour avatar HaloFour commented on June 9, 2024

@Neme12

Is it yet decided whether this will allow overloading by return type? If so, we need new reflection APIs (e.g. Type.GetMethod) that take a return type in addition to the parameter types

Technically the CLR has always allowed for overloading by return type, as the return type is a part of the method signature. I'm not aware of any languages that take advantage of the capability, though.

from csharplang.

Neme12 avatar Neme12 commented on June 9, 2024

@HaloFour Yes, I know. I'm asking if this proposal will allow overloading by return type in C# as well, with one of the overloads having a higher priority and thus always being the one that is picked. Then the need for getting a method not only by parameter types but also by return type in reflection will be greater. And it would be good to get this ability in the same .NET release.

(Although I think having the ability to specify return type in GetMethod etc in addition to parameter types is still useful and needed even without this C# feature, because IL allows it and you can encounter a method overloaded on the return type in the metadata, and I would like to always specify not only the parameter types but also the return type when calling GetMethod to specify precisely which method I want.)

from csharplang.

CyrusNajmabadi avatar CyrusNajmabadi commented on June 9, 2024

The proposal only affects overload resolution. Meaning which overload is picked for a particular call. It doesn't change at all the rules about what overloads can be declared.

from csharplang.

Neme12 avatar Neme12 commented on June 9, 2024

@CyrusNajmabadi And is there no plan to allow such overloads to be declared? From the way this feature (or the previous one - BinaryCompatOnly) was advertised and talked about by Jared and others, they mentioned the possibility of overloading by return type thanks to this feature, if I recall correctly.

from csharplang.

brianrourkeboll avatar brianrourkeboll commented on June 9, 2024

(I tried to find previous discussion of this question, but I didn't see any. If this has already been talked about somewhere, please point me to it.)

Given this bit from the proposal summary about the feature's intended use โ€”

a new attribute โ€ฆ that can be used by API authors to adjust the relative priority of overloads โ€ฆ even if those APIs would normally be considered ambiguous

(emphasis mine)

โ€” and given this example from ยง Motivation โ€”

The type or member is still visible in overload resolution, and may cause unwanted overload resolution failures when there is a perfectly good alternative, but that alternative is either ambiguous with the obsoleted member, or the presence of the obsoleted member causes overload resolution to end early without ever considering the good member. For this purpose, we want to have a way for API authors to guide overload resolution on resolving the ambiguity, so that they can evolve their API surface areas and steer users towards performant APIs without having to compromise the user experience.

(emphasis mine)

โ€” what happens when (1) an older C# compiler, (2) a C# compiler targeting an older language version, or (3) a language that does not understand this attribute encounters a library that depends on this feature?

Will it be impossible to consume an overloaded API whose only disambiguator is this attribute in such a case? Or am I misunderstanding the implications here?

from csharplang.

snarbies avatar snarbies commented on June 9, 2024

Will it be impossible to consume an overloaded API whose only disambiguator is this attribute in such a case?

At the risk of stating the obvious, such an attribute would presumably never be the only disambiguator between overloads. It only helps with the decision making when there are ambiguous non-exact matches (implicit casts). Adding explicit casts so that your invocation exactly matches a (non-binary-compat-only) overload should succeed, in C# as well as other languages.

In other words, if you can change the invoking code, you can always make it work. Without changing the code, when a library introduces back-compat-only overloads I'd expect it to risk breaking on an older compiler and for a language that does not understand the attribute.

from csharplang.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.