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

Fix GetSymbolInfo on ValueTuple declaration #43467

Merged
merged 7 commits into from
Apr 22, 2020
Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 18, 2020

@jcouv jcouv added this to the 16.7 milestone Apr 18, 2020
@jcouv jcouv self-assigned this Apr 18, 2020
@jcouv jcouv marked this pull request as ready for review April 18, 2020 00:32
@jcouv jcouv requested a review from a team as a code owner April 18, 2020 00:32
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 (iteration 1)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 18, 2020

It looks like there several more places in the code base where we perform a type check against SourceMemberFieldSymbol or a type derived from it. Please review them to make sure they do not need to be adjusted with respect to tuple fields. #Closed

@jcouv
Copy link
Member Author

jcouv commented Apr 21, 2020

Please review them to make sure they do not need to be adjusted with respect to tuple fields.

I found one. Thanks

I also realized I forgot to add a target-type new test you had suggested (here). Didn't seem worth a dedicated PR so snuck it here.

}

[Fact, WorkItem(1090920, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/1090920")]
public void NameofFixedBuffer()
Copy link
Contributor

Choose a reason for hiding this comment

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

NameofFixedBuffer [](start = 20, length = 17)

Is this test added to cover changes in MethodCompiler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it covers the handling in if (fieldSymbol.IsFixedSizeBuffer && compilationState.Emitting) that is now reachable.

{
new C(new());
new C(default);
new C(null);
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2020

Choose a reason for hiding this comment

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

This scenarios are not interesting (were not requested) because they are not using target typed new #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.

Ah, duh! My bad

Copy link
Member Author

Choose a reason for hiding this comment

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

I reacted too quickly. This test is testing target-typed new (first scenario) and comparing with default and null behavior with regards to use-site diagnostics.

I can also test C c = new(null); which will be ambiguous. Is that what you had in mind?

{
public void M2(C c)
{
new C(new());
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 21, 2020

Choose a reason for hiding this comment

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

new C [](start = 8, length = 5)

This is also not quite the scenario, the errors are not reported due to target typed new. We want the target typed new to be the only source of errors. Here we are mixing too much together. #Closed

new C(new());
new C(default);
new C(null);
C c = new(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

C c = new(null); [](start = 8, length = 16)

Please add a test for a target typed new as an argument of a call that would assert use-site errors caused by the target typed new.

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 (iteration 7)

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@jcouv jcouv merged commit df5bbbc into dotnet:master Apr 22, 2020
@ghost ghost modified the milestones: 16.7, Next Apr 22, 2020
@jcouv jcouv deleted the tuple-bug branch April 22, 2020 02:48
333fred added a commit to 333fred/roslyn that referenced this pull request Apr 23, 2020
* upstream/master: (1099 commits)
  Specially handle tuple methods in CodeGenerator.EmitMethodInfoExpression for VB. (dotnet#43553)
  Remove unnecessary usings
  AssetStorage cleanup (dotnet#43511)
  Remove unused code (dotnet#43556)
  Revert anonymous type DebuggerDisplay change (dotnet#43575)
  Replace GeneratePkgDef with impl that does not load assemblies (dotnet#43302)
  Fix
  Update src/Analyzers/Core/CodeFixes/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs
  PR feedback
  use capacity when creating builders.
  Push options down.
  Rename methods
  Fix GetSymbolInfo on ValueTuple declaration (dotnet#43467)
  Add support for cref-type-parameters.
  Support OOP with dynamic types.
  Support error locals in symbolkey
  Update tests to run OOP
  Update docs/Language Feature Status.md
  Update for partial methods
  Fix typos (dotnet#43494)
  ...
@sharwell sharwell modified the milestones: Next, temp Apr 28, 2020
@sharwell sharwell added this to the 16.7.P1 milestone Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants