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

'RegisterSymbolAction' for 'Property' doesn't run at all on partial properties with attributes on both definition and implementation parts #76166

Open
Sergio0694 opened this issue Nov 30, 2024 · 6 comments
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 30, 2024

Version Used: 4.12.0-3.24558.5 (21192bf)

Steps to Reproduce:

  1. Clone https://github.com/CommunityToolkit/dotnet
  2. Checkout dev/more-analyzers (or Add more analyzers, enable unit tests for partial properties CommunityToolkit/dotnet#1010)
  3. Remove the [Ignore] from InvalidPartialPropertyLevelObservablePropertyAttributeAnalyzer_OnImplementedProperty_GeneratedByAnotherGenerator_Warns
  4. Put a breakpoint in the RegisterSymbolAction callback in InvalidPartialPropertyLevelObservablePropertyAttributeAnalyzer
  5. Run the test
  6. Verify that the callback is never executed
  7. Comment out the [GeneratedCode] attribute on the partial property implementation in that test
  8. Run the test again
  9. Verify the callback is now hit (??!)

Expected Behavior:

The RegisterSymbolAction callback should work as expected in both cases.

Actual Behavior:

The RegisterSymbolAction callback is apparently not called at all if you leave the attribute on the implementation part.

@Sergio0694
Copy link
Contributor Author

cc. @RikkiGibson. I found this in CommunityToolkit/dotnet#1010.

@Youssef1313
Copy link
Member

Somewhat overlaps with #69543

@Youssef1313
Copy link
Member

It might not be exactly #69543.

I didn't debug, but just looking around in the code.

// GeneratedCodeAttribute can only be applied once on a symbol.
// For partial symbols with more than one definition, we must treat them as non-generated code symbols.
if (symbol.DeclaringSyntaxReferences.Length > 1)
{
return false;
}
foreach (var attribute in symbol.GetAttributes())
{
if (generatedCodeAttribute.Equals(attribute.AttributeClass))
{
return true;
}
}
}

This should not be considering GeneratedCodeAttribute if the symbol has multiple syntax refs.

However, I see this in property symbol:

public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
{
get
{
return ImmutableArray.Create(_syntaxRef);
}
}

This may be outdated code?

@RikkiGibson
Copy link
Contributor

However, I see this in property symbol

I think DeclaringSyntaxReferences is implemented as expected. It is consistent with methods. Each part symbol has a single DeclaringSyntaxReference, and you need to get that part symbol to get the appropriate SyntaxReference for it.

The symbol.DeclaringSyntaxReferences.Length > 1 seems valid for partial types, but, additional work is probably needed here to simply check "is this a partial method/property with multiple declarations."

@Youssef1313
Copy link
Member

Makes sense. Thanks @RikkiGibson

@Sergio0694
Copy link
Contributor Author

As a temporary workaround I ended up enabling the analyzer to run on generated code too, seems to do the trick 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

No branches or pull requests

3 participants