Allow passing additional symbol info in GetDeclarationListInfo #3909

auduchinok · 2017-11-08T11:53:33Z

We don't use Accessibility field in FSharpDeclarationListItem and want to pass other symbol info instead.
I added two optional parameters to GetDeclarationListInfo:

getAdditionalInfo: function to replace getAccessibility which takes FSharpSymbol and FSharpDisplayContext and allows passing arbitrary symbol info.
shortTypeNames: use short type names in the display context

KevinRansom

Nice.

Thank you

Kevin

KevinRansom · 2017-11-08T17:10:16Z

src/fsharp/vs/service.fs

+                    let getAdditionalInfo item =
+                        try getAdditionalInfo(FSharpSymbol.Create(g, thisCcu, tcImports, item), displayContext)
+                        with _ ->
+                            Trace.TraceInformation(sprintf "FCS: could not get additional info for %A" item)


It might be useful for diagnostics to trace the exception here.

dsyme · 2017-11-08T17:50:18Z

@auduchinok The use of generics like this is odd for FCS and I'd like to explore alternatives ways of getting the same result.

e.g. why not just include the FSharpDisplayContext and FSharpSymbol in each returned FSharpDeclarationListItem and let the caller augment the information post-hoc?

auduchinok · 2017-11-08T17:57:08Z

@dsyme I like this idea. Initially I've tried to make this an in-place replacement for Accessibility field and ended up with this design.

dsyme · 2017-11-08T18:08:38Z

@dsyme I like this idea.

:) You mean you like the PR as is, or with the suggestion above? Thx :)

auduchinok · 2017-11-08T18:09:19Z

The suggestion. :)

dsyme · 2017-11-08T18:16:58Z

:) thx

auduchinok · 2017-11-08T19:28:14Z

@dsyme Do you know if there was a particular reason FSharpDeclarationListInfo was written as a class? I'd like to rewrite it as a discriminated union (and keep the Create method there) as there are three situations (Empty, Error and (let's say) Info) that need completely different sets of fields.

dsyme · 2017-11-08T21:59:11Z

@dsyme Do you know if there was a particular reason FSharpDeclarationListInfo was written as a class? I'd like to rewrite it as a discriminated union (and keep the Create method there) as there are three situations (Empty, Error and (let's say) Info) that need completely different sets of fields.

I think it just grew up that way. If you feel as a consumer that you would have have avoided an error or need to evolve the three cases separately then yes, please do send the PR

That said I'm not totally sure it's worth churning the API at this point. There will be consumers in various tooling like FSharp.Formatting and Ionide. But send the PR and we can discuss

(In general using unions and records in evolving APIs is not always encouraged. There's a tradeoff here - unions and records are great accurate programming devices since they cause rigidity - and thus force accuracy on consumers as types evolve. In contrast, classes cause less rigidity - since you can just add new properties - but likewise new information can creep in that the consumer ignores. )

KevinRansom · 2017-11-13T23:46:31Z

@dsyme this looks ready to go to me. Are you satisfied?

auduchinok · 2017-11-14T08:17:53Z

@KevinRansom I'd like to change it the way we discussed it before it goes in.

dsyme

marking as changes requested as discussed

KevinRansom · 2017-12-15T20:23:52Z

@auduchinok, do you think you will be able to get to the requested changes?

Thanks

Kevin

auduchinok · 2017-12-15T20:45:22Z

@KevinRansom Yes, I was going to work on it next week.

KevinRansom · 2017-12-15T20:46:32Z

Yay ... a Christmas present then :-)

auduchinok · 2017-12-20T14:58:40Z

That said I'm not totally sure it's worth churning the API at this point. There will be consumers in various tooling like FSharp.Formatting and Ionide. But send the PR and we can discuss

@dsyme I think we can simplify APIs a lot in the way it was stated in this Vasily's comment. Looking at the comment reactions I suppose most extensive API users would be happy with such changes, so may we assume that people will be OK with adopting changed APIs then?
I'd be happy to propose such a PR if it's OK to change these core public APIs. I'd like to simplify signatures and improve the wording a bit (e.g. to change GetDeclarationListInfo to GetCompletionItems or GetF1Keword to GetHelpKeyword).
/cc @nosami @Krzysztof-Cieslak

Speaking of this PR I'd like to continue working on completion features on top of simplified APIs if the change is welcomed.

nosami · 2017-12-20T15:06:24Z

@auduchinok VS for Mac doesn't use GetDeclarationListInfo. It uses GetDeclarationListSymbols instead (for now at least).

See fsharp/fsharp-compiler-docs#841 for the API change I wanted to make. I think it's important that the two APIs share the same features.

auduchinok · 2017-12-20T15:16:36Z

@nosami Yeah, I've checked if there're real uses of GetDeclarationListSymbols and GetMethodsAsSymbols and found VS for Mac to be the only tool using them. We've moved to the non-symbol ones (at least for now) as there's ready-to-use results including the text to insert, namespace to open, etc, and getting the symbol will probably be added to the info.

nosami · 2017-12-20T15:19:02Z

@auduchinok: If the symbol was added to the completion info, then I think I would stop using GetDeclarationListSymbols :)

cartermp · 2017-12-20T19:01:46Z

+1 on changing the API names as you suggested @auduchinok

Krzysztof-Cieslak · 2017-12-20T20:37:02Z

+1 from me, on both API changed in the PR, and changing the names of the functions across the API. If we were looking into any breaking changes we could seriously review all the API and change it even more - for example, I'm not a big fan of passing idents to most functions, I kinda feel it could be hidden from the consumer (especially that we have now QuickParse module already in place)

auduchinok · 2017-12-20T20:39:29Z

@Krzysztof-Cieslak I've already removed idents passing in public APIs (in a branch) and do it in implementations.

forki · 2018-01-28T07:21:58Z

Ping

dsyme

@auduchinok I'm ok with these changes but they need to be merged with master

KevinRansom · 2018-09-12T21:20:39Z

@auduchinok, the PR has approval, if you resolve the merge issues it can be merged.

Thanks

Kevin

KevinRansom · 2018-10-02T14:44:24Z

These look like good changes to have in the product.

Please reactivate when you get some time to devote to this.

Thank you

Kevin

auduchinok added 3 commits November 8, 2017 19:09

Allow passing additional symbol info in GetDeclarationListInfo

6a5580e

FSharp.Editor, FSharp.LanguageService fixes

cfbe870

Update xmlDoc for new parameters

0dcf2d5

auduchinok force-pushed the completion-info branch from 0fd1f51 to 0dcf2d5 Compare November 8, 2017 16:10

KevinRansom approved these changes Nov 8, 2017

View reviewed changes

Trace exception in getAdditionalInfo

7cb7cee

tmp

467535b

dsyme requested changes Nov 14, 2017

View reviewed changes

dsyme approved these changes May 30, 2018

View reviewed changes

KevinRansom closed this Oct 2, 2018

Allow passing additional symbol info in GetDeclarationListInfo #3909

Allow passing additional symbol info in GetDeclarationListInfo #3909

Uh oh!

Conversation

auduchinok commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansom Nov 8, 2017

Choose a reason for hiding this comment

Uh oh!

dsyme commented Nov 8, 2017

Uh oh!

auduchinok commented Nov 8, 2017

Uh oh!

dsyme commented Nov 8, 2017

Uh oh!

auduchinok commented Nov 8, 2017

Uh oh!

dsyme commented Nov 8, 2017

Uh oh!

auduchinok commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Nov 8, 2017

Uh oh!

KevinRansom commented Nov 13, 2017

Uh oh!

auduchinok commented Nov 14, 2017

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Dec 15, 2017

Uh oh!

auduchinok commented Dec 15, 2017

Uh oh!

KevinRansom commented Dec 15, 2017

Uh oh!

auduchinok commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nosami commented Dec 20, 2017

Uh oh!

auduchinok commented Dec 20, 2017

Uh oh!

nosami commented Dec 20, 2017

Uh oh!

cartermp commented Dec 20, 2017

Uh oh!

Krzysztof-Cieslak commented Dec 20, 2017

Uh oh!

auduchinok commented Dec 20, 2017

Uh oh!

forki commented Jan 28, 2018

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Sep 12, 2018

Uh oh!

KevinRansom commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

auduchinok commented Nov 8, 2017 •

edited

Loading

auduchinok commented Nov 8, 2017 •

edited

Loading

auduchinok commented Dec 20, 2017 •

edited

Loading