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.

Sign up for GitHub

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

Jump to bottom

Give 'typeof'-like errors in attribute type arguments #54956

Merged
merged 6 commits into from
Jul 21, 2021
Merged

Give 'typeof'-like errors in attribute type arguments #54956

merged 6 commits into from
Jul 21, 2021

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jul 19, 2021 •
edited
Loading

Resolves dotnet/runtime#64655
Resolves #54778
Related to #36285

@RikkiGibson RikkiGibson marked this pull request as ready for review July 20, 2021 00:42
@RikkiGibson RikkiGibson requested review from a team as code owners July 20, 2021 00:42
@RikkiGibson RikkiGibson changed the base branch from features/generic-attributes to main July 20, 2021 00:42
@RikkiGibson RikkiGibson changed the base branch from main to features/generic-attributes July 20, 2021 00:42
@jcouv

This comment has been minimized.

Sign in to view
jcouv
jcouv reviewed Jul 20, 2021
View reviewed changes
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 1)

@jcouv jcouv self-assigned this Jul 20, 2021
jcouv
jcouv reviewed Jul 20, 2021
View reviewed changes
src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests.cs
@@ -9938,62 +9938,59 @@ static void Main()

[Fact]
[WorkItem(54778, "https://github.com/dotnet/roslyn/issues/54778")]
public void GenericAttribute_Dynamic_01()
public void GenericAttribute_AttributeDependentTypes()
Copy link
Member

Choose a reason for hiding this comment

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

Consider testing a nested situation (type is bad in multiple ways) to show that we only report once.
For example List<string?>? or (int a, (int b, int c)).

jcouv
jcouv approved these changes Jul 20, 2021
View reviewed changes
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 2)

@RikkiGibson
Copy link
Contributor Author

@jaredpar let me know if you have any concerns about the changes since your last review.

jcouv
jcouv approved these changes Jul 21, 2021
View reviewed changes
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 6)

@RikkiGibson RikkiGibson merged commit f260ccb into dotnet:features/generic-attributes Jul 21, 2021
@RikkiGibson RikkiGibson deleted the ga-typeof branch July 21, 2021 23:37
@roji
Copy link
Member

roji commented Aug 24, 2021

Just to let people know that this broke EF Core (and apparently also Orchard, /cc @sebastienros), see dotnet/efcore#25664. In our case this was the use of typeof(object?[]), which is indeed meaningless and can be rewritten to typeof(object[]), but could be said to convey more information when reading the code... Regardless, just to make sure the potential breakage is known.

@roji roji mentioned this pull request Aug 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaredpar jaredpar jaredpar approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RikkiGibson @jcouv @roji @jaredpar