IDE0002 suggests simplifying generated classes to their base class

[bzd3y]

I don't know if this is exactly a bug, but I wasn't sure what would be more appropriate.

Version Used: VS 17.11.2

Steps to Reproduce:

The steps to reproduce may be difficult, but it would involve using something with code generation that generates a class that derives from a base class. An example of that is the Android Resource class from .NET Android/MAUI, as referenced in this issue: https://github.com/dotnet/android/issues/8381

I suppose I could create a simple reproduction project if necessary, but it seems like the information in that other issue should be enough.

The .NET Android team is looking at adding the GeneratedCodeAttribute to classes they generate.

It seems like it would make sense for this analyzer to not apply the rule to uses of generated code (classes, at least) in user code. I checked out the source code for IDE0002 to see if it already paid attention to that attribute and it looks like it does not.

My reasoning for the proposal is that 1) since that class is being generated for you, you do not have control over it and 2) since it is being generated for you, the idea is that you are going to want to use that class without having to know anything about any details like what it might be derived from, let alone use that base class directly.

And it appears that the .NET Android team tried to avoid this with that same reasoning by suppressing this rule in the generated code, but that does not suppress it in the rest of the code. The analyzer could possible check for this as well, but I don't know if that makes sense.

Optionally, it may be worth considering having it skip, or be configurable to skip, simplifying to classes that start with something like an underscore, as is the case here. I think the underscore would generally indicate a class that isn't mean to be used directly and so the analyzer shouldn't suggest using it directly.

Diagnostic Id: IDE0002 "Name can be simplified"

Expected Behavior:

Skip things like generated classes (with the above attribute) and do not suggest simplifying to their base class.

Actual Behavior:

The analyzer suggests using the base class instead of the generated class.

[CyrusNajmabadi]

The model we have for generators is that they produce code you could write yourself. And then the rest of the experience operates the same way, just with that step sped up for you.

since it is being generated for you, the idea is that you are going to want to use that class without having to know anything about any details like what it might be derived from, let alone use that base class directly.

I strongly disagree with this. Even if it generated, our view is that this is code you could have or would have written yourself. As such, you would have and should know these details as they're part of the code you own and will be working with.

The experience seems correct to me.

[bzd3y]

But why exactly do you disagree? I understand the sentiment there or your disagreement in general or in a vacuum. But that seems more of a purely philosophical idea (which isn't to say invalid, it just doesn't seem relevant here). I don't see why that would conflict with what I am proposing here.

The fact of the matter is you didn't write it yourself. And more importantly, I don't think it is true that it can always be thought of as something you could or would have written, because sometimes you don't really have a choice or at least realistically much of one. There's some reason you are using the code generator in question.

In the case of that Android Resource class, I don't think I have a choice there. I think that is just how .NET Android works. Even if I do have a choice, the idea that because I could just opt out of using it means that I have to choose between doing that or dealing with any errors/warnings/messages it produces doesn't seem very compelling to me.

Like I said, I understand the sentiment, but there is also a bit of an intrinsic contradiction at a certain point in that if I write code that triggers an analyzer then I can suppress it at that location. But I can't do that for generated code.

So now I have to choose between suppressing the rule globally or just accepting that an analyzer might generate any number of messages notifying me of things that I have no control over and can do nothing about.

While it might be that none of those are the end of the world, it does seem like some low hanging fruit in the context of a tool like these analyzers that are meant to empower developers.

So, as usual, if my opinion just differs from "the model", I would ask why this couldn't just be configurable? It would be nice if a developer could choose how they want generated code to be treated, probably in general, not just for this rule.

Similarly, maybe that is just a problem with this rule. It addresses more than one thing: Class names and their members. But those can't be configured independently. If a developer likes one of them then it seems they are stuck with both.

Personally, I think there is a lot of value to member simplification. But I don't really care for "simplifying" class names. It actually seems backwards to me most of the time. It seems to hide that a more derived/specific class is being used. It also might require other namespaces usings to be added just to "simplify" the code. It also may create more fragile code because if the relationship between the classes changes, it could break things everywhere where things were "simplified" where if I was just using the most derived class, as was my intent, that is less likely to happen.

So we can look at that as an alternate/additional proposal.

These rules/analyzers are supposed to help, but this is one of those cases where they get in the way a little.

There seem to be more people struggling with this, per this issue here: https://github.com/dotnet/roslyn/issues/3705 which I think might challenge the model you are talking about here a bit. It looks like there might be some solutions I can try in that thread, but I still think my proposals are reasonable.

[CyrusNajmabadi]

In this case, it's viewed as simpler (and has been since the rule was added 15 years ago-ish) because the member is not on the derived type, and accessing through that doesn't have any actual semantic meaning (you can see that in the IL generated).

:-)

[bzd3y]

Okay, that seems kind of dismissive of everything else I just said...

So is the answer just "No" here?

[dellis1972]

Some additional context, the "generated code" in the android case it not source code. Its an assembly which we generate via IL. It happens after the compile process has completed. It was done this way to avoid having two invocations of the csc compiler. The files we need to generate this assembly do not exist at the time the main code is compiled. This is part of how the underlying android build system works.

I just wanted to add some additional context to the conversation.

[CyrusNajmabadi]

Okay, that seems kind of dismissive of everything else I just said...

So is the answer just "No" here?

I was just trying to answer the question of why the analyzer behaves this way :-)

[CyrusNajmabadi]

Some additional context, the "generated code" in the android case it not source code.

So, fwiw, this is well out of the bounds of the design of generators. They are intended to speed up writing the exact same code a user could write themselves.

The system (and tools layered on top) are designed with that in mind. :-)

[CyrusNajmabadi]

So is the answer just "No" here?

No. :-)

We'd just need to figure out the design principle that would support this use case.

[bzd3y]

Some additional context, the "generated code" in the android case it not source code. Its an assembly which we generate via IL.

Well, it is both, right? Correct me if I am wrong, but there is the _Microsoft.Android.Resource.Designer.ResourceConstant which I think is what you are talking about. I cannot modify this file in the IDE, for example.

But there is also a Resource class that gets generated to "\obj\Debug\net8.0-android__Microsoft.Android.Resource.Designer.cs" that derives from the class in that assembly but is put in my project namespace. I can** modify this file in the IDE.

So, @CyrusNajmabadi, to be clear, my issue is with the generated code that is source code that should be within the bounds of generators you were referring to, right?

We'd just need to figure out the design principle that would support this use case.

I think the design case is that we have these rules and analyzers to apply them to point out areas where our code can be improved or even fixed but they also do that in areas that we are unable to improve or fix because we didn't write the code and any changes we make will just be overwritten the next time the code is generated.

That probably doesn't manifest much because most rules apply to the location where something is implemented. This rule differs because it applies to anywhere the class is used, allowing it to easily "bleed" out.

And just to be clear, that doesn't seem like it is because the .NET Android team made a poor decision and violated the rule or anything like that. I think what they are doing is reasonable.

I just think the analyzer is objectively applying the rule incorrectly here because it is not correct to "simplify" (and it doesn't make anything simpler anyway) this generated class to its base class. It was generated for me to use but the rule is telling me not to use it.

As for whether it simplifies things, here is what happens.

If I have something like:

notificationBuilder.SetSmallIcon(Resource.Drawable.appicon);

Then if I follow the suggestion and let it fix it then the result is this:

notificationBuilder.SetSmallIcon(_Microsoft.Android.Resource.Designer.ResourceConstant.Drawable.appicon);

I don't think it is reasonable to call that simplified. Even if the namespace is added in a using and it was just ResourceConstant that still isn't simplified.

[CyrusNajmabadi]

that still isn't simplified.

I would disable this rule (or suppress this particular location). This simplification is the reason for this rule existing :-)

[bzd3y]

I would love to disable the rule, but I can't do that without also disabling simplifying the member access part of it too. Or can I?

I am suppressing it in this current location. My point is that in every case where the rule is applied to a generated class the user is going to want to suppress it, which means it would be better to just build that into the rule.

But as for this simplification, it is not a simplification. Are you not reading my comments because they are long? Adding an entire namespace, especially one that starts with an underscore to imply it is "private/internal" or whatever doesn't simplify things...

The Resource class was generated for me to use. A rule that suggests that I don't use it is objectively broken. Fine, I can suppress it there like I have done... But it is still broken, hence my issue here in trying to improve the rule by fixing it.

Let's look at this another way. You insist it really is simplifying things. But by doing that it is creating a dependency on "internal" code. So if .NET Android changes how things work internally, my code breaks. If they change that namespace or the class name, my code breaks. They generated a class for me so that won't happen. But this rule is suggesting I "simplify" things by ensuring that will happen.

[CyrusNajmabadi]

Let's look at this another way. You insist it really is simplifying things. But by doing that it is creating a dependency on "internal" code

Can you clarify this? Does the code not compile after the fixer runs?

So if .NET Android changes how things work internally, my code breaks.

That's true regardless of what the analyzer/fixer does. I think something has not been understood here. When you call "Derived.StaticMethod" the compiler literally emits a call to "BaseType.StaticMethod". (Trust me, you can check the IL to see this). So if .net android changes that, your code will be broken no matter what. :-)

If .net android really wants you to call methods only on the derived type, and wants you to be safe with them changing the impl, then they should emit methods on the derived type for you to use instead :-)

[CyrusNajmabadi]

Are you not reading my comments because they are long?

I'm reading :-)

I'm just trying to get to the crux of the issue. Specifically, it seems as if the android generator is doing things that are pretty out of the norm in terms of how it generates code. I'm very wary of doing broad changes in response to that that could then regress experiences for other users.

I am ok trying to find potential targeted solutions that narrowly would encompass this scenario.

I'm not a fan though of things like checking for underscore. Because, as you said, if android sdk changes anything (like renaming the base class to not have an underscore) it will start firing again.

[dellis1972]

I'll add some more context on what we on the android sdk are doing. The google android sdk generates "id" values for every resource (xml, png etc) that an app uses. These "id" values are used to access the underlying resources. You never use a filename everything goes through these ids.

In .net android we expose these values via a Resource class so that users can write code like this

SetContent (Resource.Layout.main);

With .net 8 there was a design change which made the Resource class more linker friendly. This design was to place all these "id" values in a seperate assembly rather than have them compiled into each assembly. This allowed all the assemblies in the app to "share" the same id values. The problem was how to deal with users upgrading their code. Given that every user of Xamarin and earlier .net android would be using the Resource class as above.

So as part of the new resource assembly generation we also produce an Intermediate file which maps the Resource class onto the new one in the assembly. This is because the new assemblies has to exist in a namespace which is consistent across all assemblies (in this case _Microsoft.Android.Resource.Designer). But the user code wa always access in the Resource class via the root namespace of the assembly.

This is for an app project called FooApp

namespace FooApp;
pubic class Resource : _Microsoft.Android.Resource.Designer.ResourceContants

or the following for a library project called FooLibrary

namespace FooLibrary;
pubic class Resource : _Microsoft.Android.Resource.Designer.Resource

We have this mapping class so we do not break existing user code.

I think the core of this issue is that the IDE0002 is trying to simplify code in this case that from a users perspective it already very simple. Worse the suggestion in this case is actually less readable.

SetContent (Resource.Layout.main);

vs

SetContent (_Microsoft.Android.Resource.Designer.ResourceContants.Layout.main);

Also because of the heavy usage in a users app you might have to use #pragma warning disable IDE0002 and #pragma warning restore IDE0002 in allot of places or over vast blocks of code. This is because this warning is useful in allot of cases. Users will be relucant to disable it completely for a project.

#pragma warning disable IDE0002
SetContent (Resource.Layout.main);
#pragma restore disable IDE0002
...
...
...
#pragma warning disable IDE0002
var view = FindViewById (Resource.Id.foo)
#pragma restore disable IDE0002

I hope this gives some background on how this system is working. Note at no point do we use a Roslyn Source generator in this process as I mented before, this is because we do not know the values for the id's until after the app has been build by C# so this is all generated by custom MSBuild tasks. I can like the code if anyone is interested.

[CyrusNajmabadi]

(Resource.Layout.main)

Could you actually emit the value onto that type (and not the base type)?

[CyrusNajmabadi]

I think the core of this issue is that the IDE0002 is trying to simplify code

From our perspective, accessing through an unnecessary derived type isn't simple. The end IL will access through the derived type, as that's what the language is actually referencing.

that from a users perspective it already very simple

Sure. If that's the case, I would say that the user and the rule are in disagreement. In that case, disabling the rule is definitely a reasonable way to go.

[CyrusNajmabadi]

Note at no point do we use a Roslyn Source generator in this process as I mented before, this is because we do not know the values for the id's until after the app has been build by C# so this is all generated by custom MSBuild tasks. I can like the code if anyone is interested.

I'm still unclear on one thing. Is the code that is generated something the user can reference from their source?

Also, have you looked into potentially using 'file' to keep symbols private to the file you generate them in?

The problem was how to deal with users upgrading their code. Given that every user of Xamarin and earlier .net android would be using the Resource class as above.

So as part of the new resource assembly generation we also produce an Intermediate file which maps the Resource class onto the new one in the assembly

@bzd3y it looks to me like you do not need to worry about breaking changes. The android sdk goes out if its way to keep compat.

So if you do decide to update the code, it should keep working.

[bzd3y]

Can you clarify this? Does the code not compile after the fixer runs?

Yes, it compiles after the fixer runs. What I mean is that my code now references the "internal" (in quotes, because it isn't actually marked as internal and can't be) IL class that they are using instead of "my" code, the generated class that they created for me to point to it.

So my code now has _Microsoft.Android.Resource.Designer.ResourceConstant in it. So if they do something like change that namespace in the internals of their process my code can now break and would have to be changed.

Obviously that probably wouldn't be a big deal and I'd just fix it. But that is also generally a best practices thing that people try to avoid, particularly with "internal" code.

And I'm not just pointing this out about this specific case, but, again, in general. It might be safe because they promise to never ever change _Microsoft.Android.Resource.Designer.ResourceConstant but that might not be true for other libraries.

That's true regardless of what the analyzer/fixer does. I think something has not been understood here. When you call "Derived.StaticMethod" the compiler literally emits a call to "BaseType.StaticMethod". (Trust me, you can check the IL to see this). So if .net android changes that, your code will be broken no matter what. :-)

No, it isn't true regardless. Because if the derived Resource class is in my code, then the compiler knows to follow that to whatever the change was because they generate the code to do that.

notificationBuilder.SetSmallIcon(Resource.Drawable.appicon); // this will always compile as long as they generate a Resource class for me that points to the correct base class
notificationBuilder.SetSmallIcon(_Microsoft.Android.Resource.Designer.ResourceConstant.Drawable.appicon); // this will not if the base class they are using changes

I understand the IL. The compiler creates that. So in the first case, it creates it by looking at Resource and just using the static method that is being accessed, which happens to be on the base class.

In the second case, it would try to do that, but there is no class by that name(space) because some part of the name(space) changed.

If .net android really wants you to call methods only on the derived type, and wants you to be safe with them changing the impl, then they should emit methods on the derived type for you to use instead :-)

I initially thought that that was how it worked: that built in Android resource identifiers were on this base type and my app specific constants went in the generated code. That would make sense. But they can't do it that way because they are static.

Now, that Resource class is marked as partial so it may be that the intention there is that I can easily extend it by using another partial class.

But that doesn't change (I think) how resource constants are accessed because those are done statically.

I'm reading :-)

Okay, fair enough. I think you just have a different style of replying in pieces, which is fine and I have seen that that is how you have replied in the past. It is just sometimes hard to tell. So I'll try to be more patient.

Specifically, it seems as if the android generator is doing things that are pretty out of the norm in terms of how it generates code.

I don't think it is. And even if it is, I can't think of another or better way for them to do it other than just having all the constants in this one generated class.

I'm very wary of doing broad changes in response to that that could then regress experiences for other users.

I don't see how that could be an issue here, especially if this were just configurable. Any exceptions within that could just be handled by suppressing the rule one way or another.

But I feel pretty strongly/confidently that nobody is going to have problems with this rule if it doesn't triggered for generated code. I can't imagine anybody would want to be bothered about code that they didn't physically write, even if it is still "their code", unless it just doesn't compile.

@bzd3y it looks to me like you do not need to worry about breaking changes. The android sdk goes out if its way to keep compat.

Yes... They do... That is the reason I pointed this out... By generating that Resource class for me that the rule is trying to get me to not use. If I follow the suggested fix then that completely undermines that effort on their part...

And, again, even if that wasn't a concern here because they promise to never ever ever ever change _Microsoft.Android.Resource.Designer.ResourceConstant (which they have not promised, should not promise and cannot promise) then this would still be a problem for other libraries that don't make that promise for their internal code.

@dellis1972 Thanks for providing more info.

[CyrusNajmabadi]

What I mean is that my code now references the "internal" (in quotes, because it isn't actually marked as internal and can't be

Could it be a 'file' type? I'd definitely be ok making the analyzer detect that and not offer in that case.

[CyrusNajmabadi]

So if they do something like change that namespace in the internals of their process my code can now break and would have to be changed.

They can't. It would be a breaking change (both source and binary). They even emit in a particular fashion to avoid breaks already.

[CyrusNajmabadi]

but that might not be true for other libraries.

It had to be. Otherwise we'd be getting breaking changes for decades. This is the first report of this issue on all that time :-)

[CyrusNajmabadi]

for them to do it other than just having all the constants in this one generated class.

They can have the constants emitted into a partial part of the Resource type. That's what other generators do :-)

[CyrusNajmabadi]

If I follow the suggested fix then that completely undermines that effort on their part...

They cannot change the base type either. As nothing blocks any existing code from using that type. As such, any breaks there would break customers. You can't make changes based on what you hope users are doing. You have to make changes given what users could be doing. :-)

[CyrusNajmabadi]

then this would still be a problem for other libraries that don't make that promise for their internal code

Which libraries are those though? We haven't gotten any reports about this from any others. And we even designed the 'file' feature to help insulate generator authors so they can generate code that they can freely change that users cannot take a dependency on.

I want to know if that tech could be used first.

I'd also be fine fixing any "simplify" bugs if we happen to improperly simplify to the "file" type :-)

[bzd3y]

It seems that all this discussion is to just avoid improving something that could be easily improved. There is no risk of breaking changes here. The rule would just be slightly less aggressive in specific situations where it shouldn't even be applied anyway.

This seems like a straightforward decision here. Is the implementation just difficult or something? There's no danger in breaking anybody's code, right? The worst outcome is that people get suggestions for unnecessary suppressions because they were suppressing an unnecessary suggestion...

Could it be a 'file' type? I'd definitely be ok making the analyzer detect that and not offer in that case.

I'm not sure what you mean by 'file' type. Do you mean extension or suffix like ".Designer.cs" or in this case ".Generator.cs"? Sure, if people want to change all their code to output that instead. That isn't up to me. I'm the consumer of the library. This seems more complicated than just doing it for generated files.

So I think the "file type" is just generated files or code, particularly when marked with the GeneratedCodeAttribute. What is the purpose of that attribute if not to indicate that the code is generated?

They can't. It would be a breaking change (both source and binary). They even emit in a particular fashion to avoid breaks already.

No, they don't. As explained above (@dellis1972 correct me if I am wrong) they emit however they want. This file and the class in it that we are talking about here are what they do to avoid breaks.

It had to be. Otherwise we'd be getting breaking changes for decades. This is the first report of this issue on all that time :-)

I don't think this is true. I don't even know how you would suggest this when the fact is that literally every single person who uses MAUI/.NET Android now, or at least that builds for Android, is susceptible to this. So most of those people are either just ignoring it, suppressing it, or just haven't encountered it yet.

And when it does get reported, that would most likely go to the library that is "causing it", and they will probably just say they can't do anything, because they can't, and close the issue, which is what almost happened here in .NET Android.

My issue here is the result of an issue that I didn't create there. So it is actually the 2nd report. And there is another person in that original thread, so you could consider it the 3rd.

The reason this is only now just popping up is probably because MAUI only recently started doing this.

If you Google this, you'd find that there is quite a bit of discussion on this topic, but most of it involves things that predate the .NET Analyzers, like Resharper, FxCop, StyleCop, etc.

They can have the constants emitted into a partial part of the Resource type. That's what other generators do :-)

As explaind above (@dellis1972 again, correct me if I am wrong) they do not know these constant values until compile time.

They cannot change the base type either.

Yes. They can.

As nothing blocks any existing code from using that type.

That isn't their problem. That is just like any other publicly visible "internal" code. EFCore, for example, does the same thing, where there is internal API that you can access with the caveat that they will break your code if they want to. And now you even get a warning or message if your code uses that internal API. And so if you really need to use it, you suppress that message.

But here we have the opposite... This analyzer is suggesting you use the internal API...

But this gives me an idea that might satisfy what you are gong for. Maybe it would be better for the rule to be made to ignore "internal" code so it doesn't suggest using it? That is really the more abstract/generalized case here. Ultimately this being generated code is irrelevant.

Would that be better? I don't know if such an attribute already exists. As far as I know, EFCore uses an EF specific attribute.

You have to make changes given what users could be doing. :-)

That is what I am proposing we do...

Which libraries are those though?

It is any library that has a public class that derives from an "internal" class were static members are used.

I'd also be fine fixing any "simplify" bugs if we happen to improperly simplify to the "file" type :-)

I guess I don't exactly understand what the "file" type thing is. It's just that whatever it is, it seems more complicated than just having the rule not suggest the user ignores a class that somebody provided for them and use an internal one that they shouldn't be using instead.

That is the problem here. A library author can suggest and expect you to use X because you can't rely on Y. This rule suggests you use Y.

[CyrusNajmabadi]

The rule would just be slightly less aggressive in specific situations where it shouldn't even be applied anyway.

That is your opinion. It is not mine. Indeed, we generate code in Roslyn, but I still want calls to statics to go through the base type even when generated.

You are taking your specific API case (android) and extrapolating out to all generated code cases.

[CyrusNajmabadi]

I'm not sure what you mean by 'file' type.

I mean the c# language 'file' feature. E.g. file class MyClass. A feature we created to allow generated code to be compiled into an assembly, without leaking out of a particular file.

[CyrusNajmabadi]

Yes. They can.

Then they are breaking compat. Binaries will break if they do this.

Off to dinner. Will respond later :-)

[CyrusNajmabadi]

they do not know these constant values until compile time.

That's fine. That's when analyzers run too.

[CyrusNajmabadi]

It is any library that has a public class that derives from an "internal" class were static members are used.

This is not possible. The language doesn't support public classes deriving from internal types.

Either both types are public, or both are internal. Or the base type is public and the derived type is internal.

The derived type can't expand visibility.

--

Now, if you're saying that the analyzer teens you to use an accessible type that you are not supposed to use (because of a strange Api design choice) then that is something outside of the language. That's not a concept of c# (or .net tbh), it's some strange android-ism, and is far out of bounds on how we recommend apis be designed.

Note: as mentioned above, we do have a language feature to help here. Specifically the c# "file" feature, which restricts symbols from being used outside of the file they are declared in.

This feature was created precisely so generators could generate symbols that users themselves should not reference.

[bzd3y]

but I still want calls to statics to go through the base type even when generated.

But why? Can you give an example of where this would be desirable? It seems to me that the fact that the file was generated means any base types should be irrelevant to the user.

I mean the c# language 'file' feature. E.g. file class MyClass.

Ah, okay, I didn't realize we were talking about a modifier. I don't think that would work because the generated class has to have access to that compiled base class, but they would have to try that. If they try it and it solves the problem then that seems fine to me.

But if that works, then that just matches up with the principles I have been pushing, that the internals of the generated class shouldn't leak out to the user.

Then they are breaking compat. Binaries will break if they do this.

I don't see how. This is why they generated the class they are generating. It points to the other file. If the file changed, then a new generator would just point to that one.

What would break is user code that is "pressured" into pointing to that class directly.

That's fine. That's when analyzers run too.

No, the point, to my understanding, is that they can't generate it before it gets compiled.

This is not possible. The language doesn't support public classes deriving from internal types.

Okay, that is maybe where the misunderstanding is? It isn't an internal class. It is an "internal" class.

Either both types are public, or both are internal. Or the base type is public and the derived type is internal.

Both types are public, because they have to be, yes. The entire point is that the base class is "internal", as in it is not intended to be used by the user.

But this rule suggests to use it.

Now, if you're saying that the analyzer teens you to use an accessible type that you are not supposed to use (because of a strange Api design choice) then that is something outside of the language.

Yes, but that isn't a "strange API design choice". That's pretty standard. EFCore does it, as I pointed out.

All that is going on is that sometimes part of an API has to be public but isn't intended to be used by the user, so it is marked as "internal" somehow. EFCore does it with a custom/specific attribute.

That's not a concept of c# (or .net tbh), it's some strange android-ism, and is far out of bounds on how we recommend apis be designed.

This seems kind of insulting. It isn't strange and it has nothing to do with Android. It's a principle that they just happened to use, which is pretty much exactly the same principle that EFCore uses, as another example. A lot of libraries do this.

Note: as mentioned above, we do have a language feature to help here. Specifically the c# "file" feature, which restricts symbols from being used outside of the file they are declared in.

I don't think that will work here. It needs to be used outside the file it is declared in because the derived class has to derive from it.

This feature was created precisely so generators could generate symbols that users themselves should not reference.

But that isn't what is happening here... Can you just go look at their code or something? I think part of the problem is a misunderstanding of what they are doing.

[CyrusNajmabadi]

It seems to me that the fact that the file was generated means any base types should be irrelevant to the user.

There's nothing to support this. Generated code doesn't mean "irrelevant to the user". Most generators that generate code generate code that is relevant to the user. If a generator generates code that is not relevant, they should use 'file' to explicitly mark it as such. This will also keep them safe from breaks by preventing users from directly referencing code they may want to change.

But if that works, then that just matches up with the principles I have been pushing, that the internals of the generated class shouldn't leak out to the user.

But that's not a principle. Most of the time users are expected to be able to use generated code. We created the 'file' feature for the rarer times when users are not expected to do this.

If this was an actual principle, we would have doc'ed it as such and told the ecosystem to code accordingly.

We have not done this because the "principle" you are stating does not exist.

I don't see how. This is why they generated the class they are generating. It points to the other file. If the file changed, then a new generator would just point to that one.

Because nothing stops the user from accessing any of the generated code (including the base type). So if they change that, it can break things.

No, the point, to my understanding, is that they can't generate it before it gets compiled.

I really have no idea what this means. Generators run during compilation. That's completely fine. It would be fine for them to generate the methods they want users to call in the derived type. This is all possible and what other normal generators do.

Both types are public, because they have to be, yes. The entire point is that the base class is "internal", as in it is not intended to be used by the user.

That's neither a c# nor .net concept. And if it is public, they cannot change it as it is part of your public binary ABI. Changing it will literally break any downstream dependencies that use these public types and methods. That's the nature of public.

I get that you say they are doing this. But I'm saying what they are doing is deeply out of line with our actual design guidelines and rules and breaking changes.

Yes, but that isn't a "strange API design choice". That's pretty standard.

I can find no standard that says to design this this way. Reading the .net breaking changes doc, this goes against all our published rules in breaking changes.

This seems kind of insulting. It isn't strange and it has nothing to do with Android. It's a principle that they just happened to use,

I don't know how it is insulting. They are using a pattern which goes against our docs about breaking changes. It doesn't match the rules of the lang or runtime. It's a domain specific choice which doesn't align with the design patterns we've documented for over 25 years. :-)

[CyrusNajmabadi]

Here's an idea. If android puts EditorBrowsble(Never)] on this type, then we can detect that and not offer to simplify to it

[CyrusNajmabadi]

EditorBrowsable is the supported mechanism we've had for around 20 years to indicate that something may be public but isn't intended to be seen directly in user code. This attribute would allow us to clearly distinguish public types intended for use by the user and types they shouldn't directly use.

[bzd3y]

There's nothing to support this. Generated code doesn't mean "irrelevant to the user". ... Most of the time users are expected to be able to use generated code.

Absolutely! And in this case the rule is suggesting they don't use it and instead use the class that it is derived from...

I think that's where the confusion is. The generated code isn't irrelevant to the user here. It is what they should be using. The rule is suggesting they don't...

When I was talking about things being irrelevant, I was talking about the "internals" (the general word, not the C# keyword) of any generated code.

I think there is a contradiction in your argument here, in that if a user is expected to take into account the internal details of the code that is generated for them, then that is exactly what would risk causing breaking changes when that code is changed.

Say they are using some code generator in a library, well if the authors of that library change the way the code is generated and the user is referencing that, then their code could break.

Because nothing stops the user from accessing any of the generated code (including the base type). So if they change that, it can break things.

The base type is not the generated code! This ultimately has nothing to do with generated code being involved other than it is how they provide a class to use to avoid breaking changes.

But, again, this rule is undermining that.

I really have no idea what this means.

I think the reasons they are doing this are somewhat irrelevant, but to my understanding, the reason is that they need the namespace to be consistent everywhere, i.e., not one of the user's namespaces. That is so other .NET Android (plus MAUI?) libraries can access these constants because their values were not available at the time that those libraries were compiled. So they create this base class at compile time with a known namespace and name. Their other libraries know it is supposed to exist and so they use it. The user doesn't really know it exists and isn't really supposed to.

So to simplify things and avoid breaking user code, especially existing code form Xamarin, they generate a "stub" class that just derives from that class that the .NET Android libraries reference.

That's neither a c# nor .net concept.

Maybe it is now... I think it is a general programming concept. Languages with no access modifiers or concept of public or private would do the same thing. In languages like JS/TS you would prefix these things with a _, and that is a pretty standard convention for "you don't need to worry about this thing and probably shouldn't be using it".

And the reason they do this here is because both classes have to be public and they have no choice.

But I'm saying what they are doing is deeply out of line with our actual design guidelines and rules and breaking changes.

I guess you could take that up with them (and the EF team who does essentially the same thing, I guess). But I think they are actually following them in that they are doing something to avoid breaking changes. The problem is your rule is suggesting users don't use that...

I don't know how it is insulting.

Well, I can't speak for them. But it seems kind of disparaging. You are calling it "strange" and "weird" and insisting it is a poor design.

Here's an idea. If android puts EditorBrowsble(Never)] on this type, then we can detect that and not offer to simplify to it. ... EditorBrowsable is the supported mechanism we've had for around 20 years to indicate that something may be public but isn't intended to be seen directly in user code.

Ah, so it is a principle after all.

But that is a good idea. I wish one of us had thought of that sooner. This is basically the "internal" attribute I was looking for.

@dellis1972 does that work? I think you could put the GeneratedCodeAttribute on the Resource class anyway, but that is up to you/your team. And EditorBrowsable(Never) could go on the ResourceConstant class? That also seems like a good idea regardless of this rule.

[CyrusNajmabadi]

and instead use the class that it is derived from...

The base class of a class is part of it and a normal part of the contract for it in c# and .net.

That's true if it's generated or not generated.

It's literally why this rule exists. Because the normal expectation for the language and compiler is that you just use the base method.

That's literally what the code will compile to. And the android sdk can't change his as it would be a binary break otherwise.

[CyrusNajmabadi]

When I was talking about things being irrelevant, I was talking about the "internals" (the general word, not the C# keyword

There's no such concept in c# or .net. A public base class is not irrelevant. It's, definitionally, one of the most important aspects of a type.

We have an attribute to hide it for when API authors have made a mistake and must keep the type for binary compat, but which they now have realized they do not want users to use. You can use that attribute here, and we would support it. But we will not change the behavior otherwise, as it would break all the normal use cases this rule was intended for.

[CyrusNajmabadi]

Say they are using some code generator in a library, well if the authors of that library change the way the code is generated and the user is referencing that, then their code could break.

Yes. That's exactly what I'm saying. I'm really not sure how to get this point across any better. This is part and parcel about how c# and .net work.

We literally have docs and docs on this. Your public types (generated or not) are part of the contract (and ABI). They can't change without breaking things.

To avoid that, we have technology like 'file'. But you have to use it. If you don't, we'll treat your code as we would treat rest of the code in the .net ecosystem. The same way we've treated it for 25 years.

[CyrusNajmabadi]

Maybe it is now...

It is not. No part of the language team or runtime team has created such a rule.

And the reason they do this here is because both classes have to be public and they have no choice.

If it's public, it's part of the ABI. This is a hard rule of .net.

[CyrusNajmabadi]

Ah, so it is a principle after all.

As I said EditorBrowsable exists for when teams make a mistake making something public that they now cannot get rid of, but they want users to avoid using.

We literally created it to help teams who ended up in a bad state. But we only give special treatment to code that has it. We don't make normal code behave weirdly as the car majority of the ecosystem is in good shape and does not need special rules.

If you want special behavior here, we would consider it if you add this attribute.

I genuinely don't have any better solution. We will not change the behavior for non-attributed code as that goes against our philosophy of c# and .net for generated code for decades.

If you want to pursue this, let us know.

[CyrusNajmabadi]

Well, I can't speak for them. But it seems kind of disparaging. You are calling it "strange" and "weird" and insisting it is a poor design.

It's a strange design to have public types that aren't intended for use, yes.

That's unlike anything in the ecosystem, and not in line with any of our documentation.

We have a single attribute that can be used when teams are unfortunately in this situation and can't make the ABI break of removing the type.

This attribute is used sparingly as a last resort. But if that's the situation the android sdk is in, then it can be used. Note: we reserve the right to treat this attribute however we want across our entire feature set if we find it in a symbol.

[bzd3y]

Okay, I tried. Closing this.

[CyrusNajmabadi]

Reopening. We haven't heard an update on if EditorBrowsable can be added by the android sdk team.

[bzd3y]

I would just close it. If they were interested in this then I think they would have joined in the discussion. It doesn't sound like many people actually want it and it really wasn't worth all that discussion or any more discussion when it can just be suppressed in code.

And I think they were probably looking at a few other possible solutions anyway and probably don't care one way or the other about changing the rule analyzer.

[dellis1972]

My understanding is that EditorBrowsable will mean that the class and any of its subclasses or properties will not show up in code completion? If that is the case it won't help because users need to be able to see what properties are available in intellisense. I could be wrong though.

I am investigating to see if I can use a DiagnosticSuppressor analyser to specificially target this particular situation. I am hoping we can ship it with the .net android sdk and have it only effect this specific senario .I'm not having much luck in getting it to work at the moment though.

[dellis1972]

actually ignore some of that last comment , we already have the EditorBrowsable attribute on the class https://github.com/dotnet/android/blob/main/src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesignerAssembly.cs#L122.

[CyrusNajmabadi]

As the class has EditorBrowsable on it, I will update this analyzer to respect that. Thanks b very much. Assigning to myself.

[bzd3y]

Well, I guess this is a different problem, but something must be going on because it definitely still shows up in the IDE. And when I tried some testing to test your concern above, everything I put it on also still showed up in the IDE.

[CyrusNajmabadi]

I haven't done the work yet to support that attribute for this feature.

[bzd3y]

I know... I am saying that they already have it on the class, but I still see it in the IDE. And anything else that I add the attribute to also still shows up in the IDE.

I'm saying there may be a problem with other things paying attention to this attribute, like Intellisense.