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

Improve GTD & simplification of anonymous type properties #5786

Closed
wants to merge 1 commit into from
Closed

Improve GTD & simplification of anonymous type properties #5786

wants to merge 1 commit into from

Conversation

dpoeschl
Copy link
Contributor

@dpoeschl dpoeschl commented Oct 8, 2015

Fixes #3589

Consider the anonymous type defined here:

var x = new { A.z }

There are two main changes:

  • SymbolFinder.FindSymbolAtPosition now prefers "A.z" over ".z" when looking up symbols at "z" in the above code. Consumers of this API will now get a consistent result between the case of "z" in "{ A.z }" and the case of "z" in "{ t = A.z }". Note: This is a behavioral change of a public API.
  • Add simplifier expansion and reduction handling of implicitly named anonymous type property declarators. A declarator such as that in "new { A.z }" is expanded to "new { A = A.z }", any relevant code fixes are performed, and then the "A = " is removed if the expression on the right is of the right kind (identifier or member access) and would cause the property to be named "A".

@dpoeschl
Copy link
Contributor Author

dpoeschl commented Oct 8, 2015

Tagging @dotnet/roslyn-ide for review (with specific @DustinCampbell callout 😄)

DustinCampbell
DustinCampbell reviewed Oct 8, 2015
View reviewed changes
src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs
return rewrittenNode;
}

return rewrittenNode.WithNameEquals(SyntaxFactory.NameEquals(implicitPropertyName).WithAdditionalAnnotations(Simplifier.Annotation, Formatter.Annotation));
Copy link
Member

Choose a reason for hiding this comment

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

total nit: Maybe break this onto two lines (placing SyntaxFactory.NameEquals on its own line) to make it clearer what the target of WithAdditionalAnnotations is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing a local for this.

@DustinCampbell
Copy link
Member

👍

@dpoeschl
Copy link
Contributor Author

Fixed up @DustinCampbell's nits, but leaving the SymbolFinder API aspect open for further discussion.

Improve GTD & simplification of anonymous type properties
1a5fee4 
Fixes dotnet#3589

Consider the anonymous type:

```C#
var x = new { A.z }
```

There are two main changes here:
 - SymbolFinder.FindSymbolAtPosition now prefers "A.z" over "<anonymous type>.z" when looking up symbols at "z" in the above code. Consumers of this API will now get a consistent result between the case of "z" in "{ A.z }" and the case of "z" in "{ t = A.z }". **Note**: This is a behavioral change of a public API.
 - Add simplifier expansion and reduction handling of implicitly named anonymous type property declarators. A declarator such as that in "new { A.z }" is expanded to "new { A = A.z }", any relevant code fixes are performed, and then the "A = " is removed if the expression on the right is of the right kind (identifier or member access) and would cause the property to be named "A".
@CyrusNajmabadi
Copy link
Member

I'm not really arguing against us having a good 'simple' method that does the right thing most of the time for most clients. :)

I think all i'm saying is that sometimes that 'simple' method is insufficient, and just leads us into the situation we're in now where features are broken in extremely subtle and hard to realize ways. It seems (to me at least), that we need to:

  1. Audit the usage of FindSymbolAtPosition to make sure we're not subtly causing problems elsewhere. I'm not at all convinced changing the default for how this worked won't cause issues elsewhere.
  2. Strongly consider and provide a better API that more richly conveys what's going on. I'm fine with that being tracked with another issue. I would then like it if that API were used more routinely in code that is trying to correctly handle the language in complex scenarios. For example, FAR/Rename would be a prime example of where we'd want to use something like this so that we could cascade properly.

Thanks!

@dpoeschl
Copy link
Contributor Author

retest this please

@davkean
Copy link
Member

davkean commented Oct 27, 2015

Be sure to update breaking change document if we go ahead with this.

@dpoeschl
Copy link
Contributor Author

dpoeschl commented Dec 6, 2015

retest vsi please

@dpoeschl
Copy link
Contributor Author

dpoeschl commented Dec 6, 2015

Changing title to test something.

@dpoeschl dpoeschl changed the title Improve GTD & simplification of anonymous type properties Code Model: GetTypeSymbolFromFullName should not return unpredictable results when the type name was in an unsupported format Dec 6, 2015
@dpoeschl dpoeschl changed the title Code Model: GetTypeSymbolFromFullName should not return unpredictable results when the type name was in an unsupported format Improve GTD & simplification of anonymous type properties Dec 6, 2015
@davkean
Copy link
Member

davkean commented Dec 7, 2015

We suspect your merge conflicts are causing the vsi failures because closed is out of sync with open, resolve them and retry.

@Vannevelj Vannevelj mentioned this pull request Jan 25, 2016
Closed
@sharwell
Copy link
Member

@dpoeschl Is this still needed? The target issue was closed recently.

@dpoeschl
Copy link
Contributor Author

Nope. Thanks.

@dpoeschl dpoeschl closed this May 15, 2017
@sharwell sharwell added the Resolution-Not Applicable The issue is not relevant to code in this repo and is not an external issue. label May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Area-IDE cla-already-signed Resolution-Not Applicable The issue is not relevant to code in this repo and is not an external issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Go To Definition on anonymous type goes to wrong definition
7 participants
@dpoeschl @DustinCampbell @CyrusNajmabadi @davkean @sharwell @jasonmalinowski @dnfclas