search

dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.08k stars 2.03k forks source link

Analyzers: Use FullyQualifiedName for attribute matching #8639

Open ledjon-behluli opened 1 year ago

ledjon-behluli commented 1 year ago

Currently the analyzers that look for attributes such as: GenerateSerializerAttribute, IdAttribute make use of the SyntaxHelpers.HasAttribute method. This calls into IsAttribute() which does the following:

  1. It uses the TryGetTypeName method to extract the name of the attribute as a string. This assumes that attributeSyntax represents a well-formed attribute.

  2. It checks if the extracted attribute name matches the provided attributeName in a case-sensitive manner using string.Equals. If it's a direct match, the method returns true.

  3. If there is no direct match, it checks if the attribute name starts with attributeName, ends with "Attribute", and has a total length equal to attributeName.Length + Attribute.Length. If these conditions are met, the method also returns true.

IMO we should not rely on the attribute name only, as users may define a similar attribute in their codebase which has the same name. For example the user may have their own: IdAttribute or it may come from 3rd-party packages (Id is a very common name).

We should rely on checking the attribute via its Fully Qualified Name, this removes potential ambiguities.

NOTE: Feel free to mark this a closed in case it is irrelevant to the source generator which XAttribute it is, as long as there is one with the proper (short)name.

image

ledjon-behluli commented 1 year ago

I am willing to work on this if deemed appropriate

ReubenBond commented 11 months ago

Checking via FullyQualifiedName is a great idea and would be most welcome, thanks @ledjon-behluli

ghost commented 11 months ago

We've moved this issue to the Backlog. This means that it is not going to be worked on for the coming release. We review items in the backlog at the end of each milestone/release and depending on the team's priority we may reconsider this issue for the following milestone.

ReubenBond commented 11 months ago

The MSFT bot is annoying in this case... I wanted to milestone the issue into some non-specific milestone

ledjon-behluli commented 11 months ago

@ReubenBond Sure no problem, but I'd rather wait until: #8643 , #8662 , #8664, #8672 are merged into main so I don't disrupt it. Afterwards I can change this and will apply to all analyzers (even the once not list in the PRs above).

Feel free to assign this to me (if you can)