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

Include file-local name prefix in names of explicit interface implementations #68331

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Include file-local name prefix in names of explicit interface implementations #68331

merged 5 commits into from
Jun 13, 2023

Conversation

RikkiGibson
Copy link
Contributor

Closes #68219

@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 26, 2023
@RikkiGibson RikkiGibson force-pushed the fix-68219 branch 2 times, most recently from b3c9dfa to 8f28ed7 Compare May 27, 2023 00:36
@RikkiGibson
Don't give duplicate member error for explicit implementations of dif…
c8889b1 
…ferent file-local interfaces with same source name
@RikkiGibson RikkiGibson changed the title Don't check member name conflicts on explicit interface implementations Don't give duplicate member error for explicit implementations of different file-local interfaces with same source name May 27, 2023
@RikkiGibson RikkiGibson marked this pull request as ready for review May 27, 2023 00:39
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 27, 2023 00:39
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for review of a fairly small bug fix

@jcouv jcouv self-assigned this May 30, 2023
jcouv
jcouv reviewed May 30, 2023
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Source/FileModifierTests.cs Outdated
}
""";

var verifier = CompileAndVerify(new[] { (source0, "F0.cs"), (source1, "F1.cs"), (source2, "F2.cs") }, verify: Verification.Skipped, expectedOutput: "12");
Copy link
Member

Choose a reason for hiding this comment

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

verify: Verification.Skipped

Why is IL verification skipped in the newly added tests (but none of the previous file-type tests did)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ILVerify incorrectly believes duplicate members are present in the tests where the code is executed.

Copy link
Member

Choose a reason for hiding this comment

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

Could you produce an IL/ildasm example for this? How is it possible given our current naming convention for file-types?
If we can confirm this is an ILVerify issue, then we should file an issue on the runtime repo.

Copy link
Contributor Author

@RikkiGibson RikkiGibson Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the issue is the name+parameter list of each member is exactly the same.

Perhaps somewhere along the way the member should be adjusted to prefix with the metadata name of the file type, instead of its source name.

FWIW, I think these members are still distinct at runtime. But it's possible that some tools which operate on IL could be confused by it.

jcouv
jcouv previously approved these changes May 30, 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 2) with a minor test question

@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label May 30, 2023
@jcouv jcouv added this to the 17.7 milestone May 30, 2023
@jcouv
Copy link
Member

jcouv commented May 30, 2023

@dotnet/roslyn-compiler for second review. Thanks

@RikkiGibson RikkiGibson requested review from a team and removed request for a team June 1, 2023 21:05
@RikkiGibson
Copy link
Contributor Author

I'll try and see if I can resolve the duplicate member issue the verifier is complaining about. If it's not too difficult, I will update this PR to just do it.

AlekseyTs
AlekseyTs requested changes Jun 2, 2023
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.

See #68331 (comment)

RikkiGibson
RikkiGibson commented Jun 2, 2023
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Source/FileModifierTests.cs Outdated
Diagnostic(ErrorCode.ERR_UnimplementedInterfaceMember, "IFoo").WithArguments("Foo", "IFoo.Bar").WithLocation(6, 30),
// F2.cs(9,14): error CS0102: The type 'Foo' already contains a definition for '<F2>F141A34209AF0D3C8CA844A7D9A360C895EB14E557F17D27626C519D9BE96AF4A__IFoo.Bar'
// int IFoo.Bar => 2;
Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "Bar").WithArguments("Foo", "<F2>F141A34209AF0D3C8CA844A7D9A360C895EB14E557F17D27626C519D9BE96AF4A__IFoo.Bar").WithLocation(9, 14));
Copy link
Contributor Author

@RikkiGibson RikkiGibson Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not ideal to expose the file type's metadata name in an error message, but it's also not a big problem.

If we decide on some way in the future to produce smaller/simpler mangled names for these types, it would improve things here among other places. For example, it's not easy to skim such type names when looking through an ETL trace.

@RikkiGibson RikkiGibson requested a review from AlekseyTs June 2, 2023 21:35
@RikkiGibson RikkiGibson changed the title Don't give duplicate member error for explicit implementations of different file-local interfaces with same source name Include file-local name prefix in names of explicit interface implementations Jun 2, 2023
AlekseyTs
AlekseyTs approved these changes Jun 6, 2023
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 3)

@RikkiGibson RikkiGibson requested a review from jcouv June 6, 2023 17:10
@AlekseyTs
Copy link
Contributor

@jcouv For the second review

jcouv
jcouv reviewed Jun 12, 2023
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/Source/FileModifierTests.cs Outdated
public void PartialExplicitImplementation_04()
{
var source1 = """
file interface IFoo
Copy link
Member

Choose a reason for hiding this comment

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

IFoo

I think we avoid "Foo" because of policheck

Copy link
Member

Choose a reason for hiding this comment

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

There's some "Foo" left here

jcouv
jcouv reviewed Jun 12, 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 3). Only minor naming issue

@RikkiGibson RikkiGibson requested a review from jcouv June 12, 2023 22:14
jcouv
jcouv reviewed Jun 12, 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 (iteration 4)

@RikkiGibson RikkiGibson requested a review from jcouv June 12, 2023 22:40
jcouv
jcouv approved these changes Jun 12, 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 merged commit f321061 into dotnet:main Jun 13, 2023
@RikkiGibson RikkiGibson deleted the fix-68219 branch June 13, 2023 00:50
@ghost ghost modified the milestones: 17.7, Next Jun 13, 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

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
17.7 P3
3 participants
@RikkiGibson @jcouv @AlekseyTs