dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.93k stars 531 forks source link

Intellisense Suggestion - name can be simplified #8381

Closed gmck closed 1 month ago

gmck commented 1 year ago

Android application type

.NET Android (net7.0-android, etc.)

Affected platform version

VS 2022 17.8.0 Prev 2.0

Description

Application type: net8.0-android.

Consider the following code snippet.

case Resource.Id.home_fragment:
case Resource.Id.gallery_fragment:
case Resource.Id.slideshow_fragment:
case Resource.Id.widgets_fragment:
case Resource.Id.purchase_fragment:

Resource. is greyed out by the editor, and IntelliSense then suggests that it can be simplified by replacing it with the following _Microsoft.Android.Resource.Designer.ResourceConstant.

While it builds and runs ok - that is a bit of a stretch to claim it is simplified.

Can we turn off a particular suggestion such as the above, without the code being wrapped in something or is internally controlled?

Steps to Reproduce

N/A

Did you find any workaround?

No

Relevant log output

No response

dellis1972 commented 1 year ago

@gmck this was already done in https://github.com/xamarin/xamarin-android/commit/3ec1b159b8125cfaea1d774da94b323f08e2432b.

I'm not sure which preview version of .net 8 you are using though, cos we did this back in January.

dellis1972 commented 1 year ago

Maybe the #pragma IDE0002 was not enough in this case.

gmck commented 1 year ago

@dellis1972

I'm not sure which preview version of .net 8 you are using though, cos we did this back in January.

I'm using the version that was released with RC1, with Version 17.8.0 Preview 2.0. Actually, how do you know which version of Net8 you are using? I just thought it was tied to the version of VS2022 and never thought about it.

I don't want to have to clutter up my code with #pragmas for Android code that has previously not given a warning. I just don't understand why with .net8 there is a warning in the first place. I briefly read through 3ec1b15 and it appears to refer to Maui, not Android.

dellis1972 commented 1 year ago

@gmck

I guess the thing to check is if the __Microsoft.Android.Resource.Designer.cs file in your obj directory has the #pragma.

This is an example of the file

//------------------------------------------------------------------------------
// <auto-generated>
//  This code was generated by a tool. DO NOT EDIT
// </auto-generated>
//------------------------------------------------------------------------------
using System;

namespace m1 {
    #pragma warning disable IDE0002
    public partial class Resource : _Microsoft.Android.Resource.Designer.ResourceConstant {
    }
    #pragma warning restore IDE0002
}

Those #pragma lines are the only way to disable the IDE0002 simplification feature afaik. Unless you disable it at a project level which you probably don't want to do.

The reason you get this warning is becuase we completely reworked the Resource.designer.cs between .net 7 and .net 8. Previously there was allot of code bloat in the Resource.desginer.cs that the linker could not remove. The new system moves all of that into an assembly which all assemblies reference (both the app and libraries) which lets the linker do its thing and remove a bunch of unused properties.

gmck commented 1 year ago

The projects __Microsoft.Android.Resource.Designer.cs is the same as your example. Therefore I shouldn't get the warning - correct? But I do "name can be simplified".

What about VS's the .editorconfig file. It has it as single entry with severity=none

dellis1972 commented 1 year ago

hmm. I wonder if you are hitting IDE0001 (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0001). not IDE0002... we might need an additional pragma there.

jpobst commented 1 year ago

IntelliSense then suggests that it can be simplified

For this point, the text used for Roslyn analyzers is static defined metadata, so it cannot be changed based on the actual context.

The one that bugs me is our style is to not use private when not needed, but the text always tells me I need to add accessibility modifiers when it actually is going to remove them:

image

dellis1972 commented 1 year ago

I'm not sure there is much we can do about this... Disabling the IDE0002 pragma in the __Microsoft.Android.Resource.Designer.cs Resource class does not do what we want. Since the _Microsoft.Android.Resource.Designer.dll is an assembly we can't put any pragma instructions in that either. We could probably use some advice from someone who knows Roslyn on how/if this is even possible to disable this for our specific use case. Since we don't want to disable it everywhere.

jpobst commented 1 year ago

Context: https://stackoverflow.com/questions/71595629/disable-ide0002-rule-in-some-cases

This is the way this Roslyn rule works, the only options we could do are probably:

We would need to testing to see if either of these actually fix the issue.

Falco20019 commented 7 months ago

I just ran into that issue as well with .NET 8.0.204 targetting net8.0-android. If I add the pragma around the location of the call, then it's gone (now only ReSharper is complaining, but IDE0002 from VS itself is gone): image

I can confirm that the pragma is on the type, but it only seems to work on the usage, not on the definition :( image

To completely get rid of it, I would now have to use this per call: image

Or doing it per class: image

But those sadly also suppress valid results and I therefore don't really like to use them :(

bzd3y commented 2 months ago

Is there any update on this?

dellis1972 commented 2 months ago

@bzd3y please read https://github.com/dotnet/android/issues/8381#issuecomment-1750783841.

There isn't much we can do since we cannot put pragma statements in an assembly. They only work on code files. I have tested adding the SuppressAttribute to the underlying Resource class in the designer assembly and all it does in suppress any warnings in that class only, not where it is being used.

This would probably need to be ignored at the IDE level's. I'm not even sure where to report it if that is the case.

bzd3y commented 2 months ago

@dellis1972 Thanks, and sorry to come back after so long. I got side tracked. I had read that, but just thought there might have been more updates. It has been a while since I was looking at this, but I think we're leaving something out of this discussion, which is to change (or get changed) the Roslyn analyzer.

I know that probably wouldn't be your team, but can you move it there? Otherwise I can make a new issue. But honestly, in that case the answer would probably be "No" and having some support from the .NET Android team would probably help. It might also require a change on the Android side anyway.

My proposal would be that the generated class (and ideally all generated classes) gets marked with GeneratedCodeAttribute. I think that makes sense anyway. It is already annotated with this comment:

//------------------------------------------------------------------------------
// <auto-generated>
//  This code was generated by a tool. DO NOT EDIT
// </auto-generated>
//------------------------------------------------------------------------------

But I don't see a reason it couldn't be annotated with that attribute.

Then the Roslyn analyzer could be made (I realize that might require a separate issue in that repository) to ignore anything with a GeneratedCodeAttribute (or, ideally, be configurable to do that or not) because I think it is reasonable that in the majority of the time you are using a class that was generated for you, you would not want to simplify to something else.

I mentioned this proposal in a duplicate issue that I created before finding this one, but I hadn't mentioned it here.

dellis1972 commented 2 months ago

The GeneratedCode suggestion is a good one. Those classes should have had that on in the first place. I've added it here https://github.com/dotnet/android/pull/9310.

As for Roslyn, I would open an issue over on that repo. You never know they might implement something. But we've done all we can from our end (once https://github.com/dotnet/android/pull/9310 is merged).

bzd3y commented 2 months ago

Okay, thanks! I'll try my luck over there.

bzd3y commented 1 month ago

@dellis1972 FYI, the answer was "No", so in that case I don't know if there is any point in you guys adding that attribute.

And actually, going by the reasoning I was given for the "No", I would say you actually shouldn't add it because it would contradict the reasoning behind code generators like this.

But thanks for taking a look at this.

bzd3y commented 1 month ago

@dellis1972 I don't want to muddy up that other discussion about the analyzer/rule side of this, but I thought of using global usings and I realized that was already suggested in this thread here https://github.com/dotnet/android/issues/8381#issuecomment-1753974196 with the note that it would need to be tested.

So I did some brief testing and a global using does seem like it allows access to that class, avoids this rule, and possibly involves less overhead since there is no actual "stub" class?

One problem it might have is that there is now no namespace associated with it at all and it is available everywhere (at least in Android platform code). I don't know how much of an issue that is.

Another potential problem (or perhaps a side-effect of the first one) is name collisions. But this already happens between this class and the Resource class in the Android namespace anyway.

Lastly, I don't know if this would work "end to end", in that I tested it by modifying the generated file to remove the class and add a global using, but I don't know if that fully simulates how it would work through a full build. With that being said, when I tried building the project it built successfully.

So the generated code might look like this:

global using Resource = _Microsoft.Android.Resource.Designer.ResourceConstant; // map exactly like it is now

Optionally, it could also generate:

global using Resource = _Microsoft.Android.Resource.Designer.ResourceConstant; // map exactly like it is now
global using AndroidResource = Android.Resource; // I think this could also be included to just make it easier for users to reference this without worrying about name collisions

I'm not even sure if the Android.Resource class is one that people generally use. Honestly, I'm not entirely certain of its role other than it is a class in the Android SDK and so it has a mapping here in .NET Android. But the code that I inherited did use that (I don't know if it should be, though...) and so this alias helped resolve the name collisions. It could certainly be something besides AndroidResource if that still seems confusing, or, as I said, left out entirely.

Anyway, if there was any interest in using the global usings, I figured it was worth noting that it does look like it would work.

JakeYallop commented 1 month ago

Could you use a DiagnosticSuppressor to solve this issue?

bzd3y commented 1 month ago

@JakeYallop I don't know, I have no experience with that. That might be a good suggestion, but ideally, I don't think this is something that should cause developers to have to do anything special.

But I can also accept that things aren't always ideal, especially my idea of it. So for now I'm fine with just suppressing it with #pragma. I just thought it was something that could be improved, either here, in the analyzer or both.

dellis1972 commented 1 month ago

The Diagnostic Suppressor might be something we can ship with .net android for this specific case. I will look into it. Thanks @JakeYallop

dellis1972 commented 1 month ago

We added a diagnostic suppressor, however this will only be available when using API 35 since the suppressor has to be part of the framework nuget package. Should be available in .net 9.