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

[feature/damAnalyzer] Enable a few linker DAM tests for analyzer #2307

Merged
merged 7 commits into from
Oct 11, 2021

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Oct 5, 2021

This turns on a few of the basic testcases for DAM analysis that don't require dataflow. I changed the signature format slightly because Roslyn formats methods with spaces separating parameters by default. I also changed the field signature format to use a format consistent with methods.

Also add a few testcases demonstrating an issue uncovered in some of the
tests from MethodReturnParameterDataFlow.
SymbolDisplayMemberOptions.IncludeExplicitInterface,
parameterOptions: SymbolDisplayParameterOptions.IncludeType
);

public static string GetDisplayName (this ISymbol symbol)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have an interface in ILLinkShared to make clearer the fact that things deriving from IMetadataTokenProvider or ISymbol should implement GetDisplayName? This also makes me wonder if we should share tests in GetDisplayNameTests (unrelated to this PR though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to share more code and tests! I gave it some thought but don't see a great way to share more of the display name code (or even interfaces). Even if we could implement interfaces for foreign types we'd need to specialize the implementation for different IMetadataTokenProvirders since the interface doesn't provide enough information to format a display name. Ideas welcome!

Comment on lines +124 to +128
public static string GetStringFromExpression (TypeOfExpressionSyntax expr, SemanticModel semanticModel, SymbolDisplayFormat displayFormat)
{
var typeSymbol = semanticModel.GetSymbolInfo (expr.Type).Symbol;
return typeSymbol?.ToDisplayString (displayFormat) ?? throw new InvalidOperationException ();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add displayFormat as an optional parameter to the bellow method and put these two lines there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made them separate because I don't think there's an obvious way to give a string representation of an expression in general. I'd rather limit the below method to expressions which are building up strings from literals, and have separate variants used to format types (or maybe other kinds in the future).

Copy link
Contributor

@mateoatr mateoatr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just minor comments.

- Skip failing tests
@sbomer sbomer merged commit 6e0ff21 into dotnet:feature/damAnalyzer Oct 11, 2021
@sbomer sbomer mentioned this pull request Jan 13, 2022
4 tasks
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…net/linker#2307)

* Format parameters with spaces

* Fix field formatting an DAM tests

* Turn on a few linker DAM tests for analyzer

* Add missing using

* Fix formatting

* FIx typo and enable more tests

Also add a few testcases demonstrating an issue uncovered in some of the
tests from MethodReturnParameterDataFlow.

* PR feedback

- Skip failing tests

Commit migrated from dotnet/linker@6e0ff21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants