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

More generic attributes tests #55202

Merged
merged 10 commits into from
Aug 11, 2021

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jul 28, 2021

Related to test plan #36285
Related to #55190

This adds coverage for everything considered blocking. Note that there is an issue when an attribute constructor parameter is of a type parameter type in its original definition, even when the parameter is a closed type in its usage. Currently I think this might be a runtime bug but we will have to dig in more closely to be sure.

@RikkiGibson RikkiGibson marked this pull request as ready for review July 29, 2021 19:32
@RikkiGibson RikkiGibson requested a review from a team as a code owner July 29, 2021 19:32
@RikkiGibson RikkiGibson added the Test Test failures in roslyn-CI label Jul 29, 2021
@@ -2108,6 +2108,22 @@ static void Main(string[] args)
Assert.NotNull(symbolInfo.Symbol);
}

// TODO: it's not clear that this test is meaningful.
Copy link
Member

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, test plan suggests including a LookupSymbols test, but I'm genuinely not sure what would be meaningful here. @333fred in case you have any suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Have a test with a constructor that takes a parameter of type T, and make sure that you can get constructor information (I can't remember whether you need to use GetSymbolInfo or LookupSymbols for that.

Copy link
Member

Choose a reason for hiding this comment

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

For LookupSymbols we'd want a test like LookupConstrAndDestr with Attr1</*bind*/string/*bind*/> to confirm we can tell the IDE symbols are in scope there.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 6)

@jcouv jcouv self-assigned this Jul 30, 2021
@jcouv jcouv marked this pull request as draft August 9, 2021 19:46
@jcouv
Copy link
Member

jcouv commented Aug 9, 2021

Marking as draft for now to make room in the review queue. Ping when ready for another look

@RikkiGibson RikkiGibson marked this pull request as ready for review August 10, 2021 20:38
var node = tree.GetRoot().DescendantNodes().OfType<AttributeSyntax>().Single();
var symbol = model.GetSymbolInfo(node);
Assert.Equal("Attr1<System.String>..ctor(System.String t)", symbol.Symbol.ToTestDisplayString());
}
Copy link
Member

Choose a reason for hiding this comment

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

Curious: have you tried a Attr1<Attr1<string>>?

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 8). Just one remaining suggested test to add, thx

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 9)

@RikkiGibson RikkiGibson merged commit 2bf65c5 into dotnet:features/generic-attributes Aug 11, 2021
@RikkiGibson
Copy link
Contributor Author

Integration test failures are not related to these changes so just merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants