Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Analyzers: Use FullyQualifiedName for attribute matching #8639

Open
ledjon-behluli opened this issue Sep 19, 2023 · 5 comments
Open

Analyzers: Use FullyQualifiedName for attribute matching #8639

ledjon-behluli opened this issue Sep 19, 2023 · 5 comments

Comments

@ledjon-behluli
Copy link
Contributor

ledjon-behluli commented Sep 19, 2023

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

@ghost ghost added the Needs: triage 🔍 label Sep 19, 2023
@ledjon-behluli
Copy link
Contributor Author

I am willing to work on this if deemed appropriate

@ReubenBond
Copy link
Member

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

@ghost
Copy link

ghost commented Nov 7, 2023

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

ReubenBond commented Nov 7, 2023

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

@ReubenBond ReubenBond changed the title Analyzer's attribute finding approach Analyzers: Use FullyQualifiedName for attribute matching Nov 7, 2023
@ledjon-behluli
Copy link
Contributor Author

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants