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

Support unbound generic types in 'nameof' operator. #75368

Merged
merged 51 commits into from
Nov 19, 2024
Merged

Support unbound generic types in 'nameof' operator. #75368

merged 51 commits into from
Nov 19, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Oct 3, 2024 •
edited
Loading

Championed proposal: dotnet/csharplang#8480

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 3, 2024
@jnm2 jnm2 mentioned this pull request Oct 3, 2024
Closed
6 tasks
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review November 4, 2024 21:30
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner November 4, 2024 21:30
@CyrusNajmabadi CyrusNajmabadi changed the title WIP: Support unbound generic types in 'nameof' operator. Support unbound generic types in 'nameof' operator. Nov 4, 2024
jcouv
jcouv reviewed Nov 14, 2024
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 17)

AlekseyTs
AlekseyTs reviewed Nov 19, 2024
View reviewed changes
src/Compilers/CSharp/Portable/Binder/OpenTypeVisitor.cs
Visit(node.ElementType);
}

public override void VisitPointerType(PointerTypeSyntax node)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

VisitPointerType

Should VisitFunctionPointerType be overridden as well? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. @333fred do function pointers need special handling here?

Copy link
Member Author

Choose a reason for hiding this comment

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

note: i've added some typeof and nameof tests taht exercise func-ptrs. The results seem expected to me. But i wouldn't mind SMEs here validating.

Copy link
Member

Choose a reason for hiding this comment

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

I've played with the change a bit and looked through the logic, and I can't see a scenario where it should matter; since we use DefaultVisit for the function pointer, no nested generic types will be marked as allowed. And for any containers, it seems that the following should always hold:

  • The container is a generic type, and we won't parse that as an unbound generic type if there are any arguments (ie, IsUnboundGenericName won't be true for something like Dictionary<,delegate*<List<>>)
  • The container is an array or nullable type, and that's always marked as seen constructed.
  • The container is a tuple type, and that's never visited by this visitor, so nothing underneath is allowed to be open.

I think that covers all the possibilities, so I don't expect there to be any issues. However, @CyrusNajmabadi, please add a test with 3 generics used like this:
C<, delegate*<int>, List<>>

Copy link
Member Author

Choose a reason for hiding this comment

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

@333fred tests added. test results seemed sensible to me, so i'm moving forward with submitting this. tnx.

@CyrusNajmabadi
Copy link
Member Author

It feels like we would want to have similar relaxation in VB

Seems like it would be nice there as well. If someone wants to champion that, i'd support it.

@AlekseyTs
Copy link
Contributor

It feels like we would want to have similar relaxation in VB

Seems like it would be nice there as well. If someone wants to champion that, i'd support it.

If you are not planning to adjust VB, please open an issue to follow up and CC @KathleenDollard on it.

@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Nov 19, 2024
Open
@CyrusNajmabadi
Copy link
Member Author

Opened #75975

AlekseyTs
AlekseyTs approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 49)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 19, 2024 •
edited
Loading

@CyrusNajmabadi Please squash commits while merging.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) November 19, 2024 19:02
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Nov 19, 2024
Merged
@CyrusNajmabadi CyrusNajmabadi merged commit 6fb1f23 into dotnet:main Nov 19, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 19, 2024
@CyrusNajmabadi CyrusNajmabadi deleted the openTypes branch November 19, 2024 20:11
CyrusNajmabadi added a commit that referenced this pull request Nov 21, 2024
@CyrusNajmabadi
Add fixer to convert from bound generic types in nameof to unbound ty…
d7b111f 
…pes. (#75976)

Followup to #75368. Adds an
analyzer/fixer to move to the new simpler form.
@dotnet-bot dotnet-bot mentioned this pull request Nov 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred left review comments

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
Next
Development

Successfully merging this pull request may close these issues.
4 participants
@CyrusNajmabadi @AlekseyTs @333fred @jcouv