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

Add "Add type annotation to parameter" refactoring + remove dangling file lost in recent refactorings #10937

Merged
merged 14 commits into from
Mar 13, 2021

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jan 25, 2021

Quick sample of it so far:

image

image

  • There are several TODOs to work out here before this is reviewable
  • Tests

@cartermp cartermp changed the title basics Add type annotation refactoring Jan 25, 2021
@cartermp cartermp marked this pull request as ready for review January 25, 2021 21:54
@cartermp cartermp changed the title Add type annotation refactoring Add "Add type annotation to parameter" refactoring Jan 25, 2021
@cartermp
Copy link
Contributor Author

A note:

This only applies to parameters for now. Why? Because of GetSymbolUseAtLocation.

GetSymbolUseAtLocation constructs an FSharpSymbolUse on the fly rather than grabbing existing ones in our name resolution sink. Because of this, it also encodes an assumption that the ItemOccurence of the FSharpSymbolUse is Use. This is actually incorrect when the FSharpSymbolUse in question is a let-bound value or parameter. The ItemOccurence for these should be Binding. But it's not.

There are three alternatives I see here:

  1. Use GetAllUsesOfAllSymbolsInFile and filter by range. This gives us the correct FSharpSymbolUse and then lets us apply this to all kinds of Binding.
  2. Call GetSymbolAtLocation and then pass it to a call of GetUsesOfSymbolInFile and filter by range.
  3. Change GetSymbolUseAtLocation to pull data from the symbol use cache we have. However, this cache is just a TcSymbolUseData [][], so this would be just as inefficient as if we just called GetAllUsesOfAllSymbolsInFile. So doing this would imply that we change our cache of these objects to allow indexing by range or some other thing.

I don't really want to entertain any of them in this PR. I think this PR accomplishes the majority use case anyways.

@cartermp cartermp force-pushed the add-type-annotation branch from 20fdc36 to dbc7407 Compare March 11, 2021 23:23
@cartermp cartermp changed the title Add "Add type annotation to parameter" refactoring Add "Add type annotation to parameter" refactoring + remove dangling file lost in recent refactorings Mar 12, 2021
@cartermp cartermp force-pushed the add-type-annotation branch from b5d08dd to 7502dcd Compare March 12, 2021 01:44
@cartermp cartermp added this to the 16.10 milestone Mar 12, 2021
Co-authored-by: Chet Husk <baronfel@users.noreply.github.com>
Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks good to me. The tests are great.

@cartermp cartermp merged commit 418d37a into dotnet:main Mar 13, 2021
@cartermp cartermp deleted the add-type-annotation branch March 13, 2021 15:34
@runfoapp runfoapp bot mentioned this pull request Mar 22, 2021
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
…file lost in recent refactorings (dotnet#10937)

Co-authored-by: Chet Husk <baronfel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants