Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Intellisense Suggestion - name can be simplified #8381

Closed
gmck opened this issue Sep 28, 2023 · 21 comments
Closed

Intellisense Suggestion - name can be simplified #8381

gmck opened this issue Sep 28, 2023 · 21 comments
Assignees
Labels
Area: App+Library Build Issues when building Library projects or Application projects.

Comments

@gmck
Copy link

gmck commented Sep 28, 2023

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

@gmck gmck added Area: App+Library Build Issues when building Library projects or Application projects. needs-triage Issues that need to be assigned. labels Sep 28, 2023
@dellis1972
Copy link
Contributor

@gmck this was already done in 3ec1b15.

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

@dellis1972 dellis1972 removed the needs-triage Issues that need to be assigned. label Sep 28, 2023
@dellis1972
Copy link
Contributor

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

@gmck
Copy link
Author

gmck commented Sep 28, 2023

@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
Copy link
Contributor

@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
Copy link
Author

gmck commented Sep 29, 2023

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
Copy link
Contributor

dellis1972 commented Sep 29, 2023

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
Copy link
Contributor

jpobst commented Oct 2, 2023

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
Copy link
Contributor

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
Copy link
Contributor

jpobst commented Oct 9, 2023

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:

  • Rename the generated class _Microsoft.Android.Resource.Designer.ResourceConstant to not contain Resource?
  • Using some sort of global using that somehow tells the compiler what specific type we want to use?

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

@Falco20019
Copy link

Falco20019 commented Apr 11, 2024

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
Copy link

bzd3y commented Aug 30, 2024

Is there any update on this?

@dellis1972
Copy link
Contributor

@bzd3y please read #8381 (comment).

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
Copy link

bzd3y commented Sep 17, 2024

@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 added a commit to dellis1972/xamarin-android that referenced this issue Sep 18, 2024
Context dotnet#8381

We generate a Resource Designer assembly and an Intermediate
source file at build time. Both of these contain classes which
should have the `GeneratedCode` Attribute. So lets add it.
The version will be the same as the build assembly used to generate it.
@dellis1972
Copy link
Contributor

The GeneratedCode suggestion is a good one. Those classes should have had that on in the first place. I've added it here #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 #9310 is merged).

@bzd3y
Copy link

bzd3y commented Sep 18, 2024

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

@bzd3y
Copy link

bzd3y commented Sep 19, 2024

@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
Copy link

bzd3y commented Sep 20, 2024

@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 #8381 (comment) 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
Copy link

Could you use a DiagnosticSuppressor to solve this issue?

@bzd3y
Copy link

bzd3y commented Sep 20, 2024

@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
Copy link
Contributor

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

dellis1972 added a commit to dellis1972/xamarin-android that referenced this issue Sep 27, 2024
Context dotnet#8381

We generate a Resource Designer assembly and an Intermediate
source file at build time. Both of these contain classes which
should have the `GeneratedCode` Attribute. So lets add it.
The version will be the same as the build assembly used to generate it.
dellis1972 added a commit to dellis1972/xamarin-android that referenced this issue Sep 30, 2024
Context dotnet#8381

We generate a Resource Designer assembly and an Intermediate
source file at build time. Both of these contain classes which
should have the `GeneratedCode` Attribute. So lets add it.
The version will be the same as the build assembly used to generate it.
jonathanpeppers pushed a commit that referenced this issue Sep 30, 2024
…IDE (#9310)

Context: #8381

Add a `DiagnosticSuppressor` to "turn off" the `IDE0002` style
diagnostic message which incorrectly tells users to use the
`_Microsoft.Android.Resource.Designer.ResourceConstant` type directly.
This can cause allot of annoyance with our users because it appears on
EVERY single usage of `Resource.*`. So you end up with what looks like
code spagetti. 

So we need to start shipping an `Analyzer` assembly along with the
`Ref` framework pack. This is the place these things need to go.
Unfortunately it means that the older frameworks will not get this
analyzer. Only the current one. 

On the packaging side, the Analyzer assembly has to go in a
`analyzers/dotnet/<language>` folder in the .Ref Nuget Package. There
also needs to be an entry in the `FrameworkList.xml` file which has a
`Type="Analyzer" ` and a `Language="cs"`. This allows the IDE's to
pickup the code. We can ship both regular Analyzers and the
DiagnosticSuppressors in the same assembly. So we can extend this with
more if needed.

How this works is we use Rosyln to look for the IDE0002 diagnsotic
message, we then check the code and see if it is refering to a
`_Microsoft.Android.Resource.Designer.*` derived class. If it is , we
add a suppression. This will stop the hint appearing in the IDE. I
have tested this on VS in devbox and it appears to work . 

Also we generate a Resource Designer assembly and an Intermediate
source file at build time. Both of these contain classes which should
have the `GeneratedCode` Attribute. So lets add it. The version will
be the same as the build assembly used to generate it.
jonathanpeppers pushed a commit that referenced this issue Sep 30, 2024
…IDE (#9310)

Context: #8381

Add a `DiagnosticSuppressor` to "turn off" the `IDE0002` style
diagnostic message which incorrectly tells users to use the
`_Microsoft.Android.Resource.Designer.ResourceConstant` type directly.
This can cause allot of annoyance with our users because it appears on
EVERY single usage of `Resource.*`. So you end up with what looks like
code spagetti.

So we need to start shipping an `Analyzer` assembly along with the
`Ref` framework pack. This is the place these things need to go.
Unfortunately it means that the older frameworks will not get this
analyzer. Only the current one.

On the packaging side, the Analyzer assembly has to go in a
`analyzers/dotnet/<language>` folder in the .Ref Nuget Package. There
also needs to be an entry in the `FrameworkList.xml` file which has a
`Type="Analyzer" ` and a `Language="cs"`. This allows the IDE's to
pickup the code. We can ship both regular Analyzers and the
DiagnosticSuppressors in the same assembly. So we can extend this with
more if needed.

How this works is we use Rosyln to look for the IDE0002 diagnsotic
message, we then check the code and see if it is refering to a
`_Microsoft.Android.Resource.Designer.*` derived class. If it is , we
add a suppression. This will stop the hint appearing in the IDE. I
have tested this on VS in devbox and it appears to work .

Also we generate a Resource Designer assembly and an Intermediate
source file at build time. Both of these contain classes which should
have the `GeneratedCode` Attribute. So lets add it. The version will
be the same as the build assembly used to generate it.
@dellis1972
Copy link
Contributor

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.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Projects
None yet
Development

No branches or pull requests

7 participants