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

Fix HasFileLocalTypes check for constructed types #68386

Merged
merged 9 commits into from
Jun 9, 2023
Merged

Fix HasFileLocalTypes check for constructed types #68386

merged 9 commits into from
Jun 9, 2023

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented May 30, 2023 •
edited by jcouv

Closes #67208

Reviewed usages of HasFileLocalTypes and tried to add tests exercising each one.

Relates to file-types: #60819

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 30, 2023
@RikkiGibson RikkiGibson marked this pull request as ready for review May 31, 2023 18:40
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 31, 2023 18:40
@jcouv
Copy link
Member

jcouv commented May 31, 2023

Reviewed usages of HasFileLocalTypes and tried to add tests exercising each one.

I would recommend each test to have a type check (either an Assert(... is X) or a cast to type X) that makes it explicit which type is being covered and the type name is visible/searchable. That makes it easier for reviewers to find which tests cover which scenarios.
I've been doing that for extensions tests and found it useful (for example, easy to find which tests cover retargeting).

@jcouv jcouv self-assigned this May 31, 2023
@RikkiGibson
Copy link
Contributor Author

I would recommend each test to have a type check

Are you suggesting the test obtains the relevant type symbols and checks their types?

@jcouv
Copy link
Member

jcouv commented May 31, 2023

Are you suggesting the test obtains the relevant type symbols and checks their types?

That's what I was suggesting, but looking at the test file it looks like it's mostly doing that already (type check and check for IsFileLocal). So I guess that wouldn't have prevented the issue...

jcouv
jcouv reviewed May 31, 2023
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)

@RikkiGibson RikkiGibson requested a review from jcouv June 1, 2023 04:54
jcouv
jcouv reviewed Jun 1, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
@@ -3392,7 +3392,7 @@ public override void VisitNamespace(NamespaceSymbol symbol)
public override void VisitNamedType(NamedTypeSymbol symbol)
{
Debug.Assert(symbol.ContainingSymbol.Kind == SymbolKind.Namespace); // avoid unnecessary traversal of nested types
if (symbol.AssociatedFileIdentifier is not null)
if (symbol.IsFileLocal)
Copy link
Member

Choose a reason for hiding this comment

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

Nice. I think this reads better/clearer :-)

jcouv
jcouv reviewed Jun 1, 2023
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 4)

@RikkiGibson RikkiGibson requested a review from jcouv June 1, 2023 18:36
jcouv
jcouv approved these changes Jun 1, 2023
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 5) with a minor suggestion in PENamedTypeSymbol

@RikkiGibson RikkiGibson requested a review from a team June 1, 2023 21:05
jcouv
jcouv approved these changes Jun 2, 2023
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 5)

@RikkiGibson RikkiGibson requested a review from a team June 2, 2023 17:45
@AlekseyTs
Copy link
Contributor

Looking

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5).

AlekseyTs
AlekseyTs approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment •
edited

Choose a reason for hiding this comment

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

LGTM (commit 6), assuming CI is passing

@RikkiGibson RikkiGibson merged commit 20f6ef9 into dotnet:main Jun 9, 2023
27 checks passed
@RikkiGibson RikkiGibson deleted the fix-67208 branch June 9, 2023 23:33
@ghost ghost added this to the Next milestone Jun 9, 2023
@allisonchou allisonchou mentioned this pull request Jun 16, 2023
Closed
@RikkiGibson RikkiGibson modified the milestones: Next, 17.7 P3 Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv 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
17.7 P3
Development

Successfully merging this pull request may close these issues.

Compiler fails to detect a constructed file type.
3 participants
@RikkiGibson @jcouv @AlekseyTs