Trigger completion optimistically #2594

vasily-kirichenko · 2017-03-12T13:08:25Z

I've tested it on TypeChecker.fs. No UI freezes at all (but completion appeared after ~2-3 seconds delay, however I expect it to be exactly 1 second, but it's another issue).

(no Ctrl+Space were used)

saul · 2017-03-12T15:26:12Z

Glad to see this revived and working well :) this is great.

KevinRansom

This looks great

majocha · 2017-03-12T20:18:27Z

Awesome, now it only needs more logic to filter out incorrect suggestions based on syntax, for example after let.

vasily-kirichenko · 2017-03-12T20:48:59Z

Yeah, it should not show anything right after let and let! However... What about let (Case1 x) = ...? It should show Case1.

majocha · 2017-03-12T20:55:37Z

Yes, it would be perfect to include only correct destructuring constructs. In C# it seems there is a lot of logic of this kind to make the experience fluid.

cartermp · 2017-03-12T23:06:04Z

@majocha That's correct, there is a lot of logic in there.

Completion for destructuring is tricky. It would definitely be annoying if I had completion trigger for the following:

let (x

And my intention was to use pattern matching to form a tuple, with one of the names being x. Perhaps constraining it only to upper-case? I'm not sure.

vasily-kirichenko · 2017-03-13T05:13:40Z

@cartermp or you may want to write something like let (module1.Module2.du1.Case1 x) = ...

In short, in F# we can restrict completion list after dot only.

cartermp · 2017-03-13T05:49:22Z

Certainly some details to work out. I'll see if I can write anything useful down in #2432

vasily-kirichenko · 2017-03-13T07:24:14Z

@cartermp that thread is about what to show in completion. The problem we are discussing in this PR is when to auto trigger completion. Those are completely different things.

Currently we use simple parsing (was the last entered char a dot? is it a single dot? etc) and the lexer (are we inside a comment or excluded block? etc).

To provide good auto-triggering experience (i.e. non obtrusive), we'd have to use parse tree and a special worker, similar to the one used in filling the completion list (are we at argument position? are we in record construction? etc), but the logic would be different.

So I think we should do not trigger completion anywhere, other then few very specific places in parse tree, which should be formalized. The first obvious place is on dot. Please, suggest other places.

vasily-kirichenko · 2017-03-13T07:46:34Z

@cartermp @Pilchie could you please help to find the code in C# editor which is responsible for triggering completion? All these http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Completion/CommonCompletionProvider.cs,54b4ce43ce7c9a05,references overrides do not have any real logic.

vasily-kirichenko · 2017-03-13T08:06:10Z

I've played with C# editor a bit. Completion triggers all the time. The only place that this PR's behavior is different is after let vs after var (C# do not trigger nor suggest anything at this point).

majocha · 2017-03-13T08:14:04Z

Another that comes to mind is after type. Generally in any place you may be declaring a new identifier completion should be up to the user.

vasily-kirichenko · 2017-03-13T08:29:46Z

@majocha Yeah. The problem here is that completion list is not empty at type | and member __.Foo(| (for instance) positions.

…optimistically Conflicts: vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs

…optimistically

vasily-kirichenko · 2017-03-13T14:31:26Z

I turned off completion at the following points:

type |
type M|
let |
let f|
let f (|
let f(a|
let f(a, |
let f(a, b: int, |, c)
...
member __.M(|
member __.M(a, b|: int, c: int)
...
Please test for usability.

cartermp · 2017-03-13T15:25:33Z

@vasily-kirichenko Here's where I'd look: https://github.com/dotnet/roslyn/tree/master/src/Features/CSharp/Portable/Completion

I haven't pulled it down yet (not at my desktop), but my only other suggestion for not offering completion would be after the . in members:

member __.| // No completion

Also, I'm not sure if this is the PR for it, but we should also have preselection for locals that are in scope.

nosami · 2017-03-13T15:37:29Z

Some more cases that are accounted for in XS

let! |
override |
for |
[1..|
module |
(fun |
1u|

Not sure if you have these covered. XS doesn't handle record labels but it probably should. Anywhere where you can type an identifier IMO should not offer completions. Some tests here

vasily-kirichenko · 2017-03-13T16:21:47Z

@nosami I think all these covered, except let x =|. Why do you not show completion in that case?

vasily-kirichenko · 2017-03-13T16:23:23Z

@cartermp it does not offer completion at the point you mention. I think it does not in master, too.

nosami · 2017-03-13T16:25:17Z

@vasily-kirichenko good question - no idea!

…optimistically

vasily-kirichenko · 2017-03-14T19:05:01Z

OK, I added a property page and a setting to turn auto triggering on and off. It's on by default. Should we make it disabled? If no, then this PR is ready.

smoothdeveloper · 2017-03-14T19:05:55Z

If we have setting, lets keep it on :) Thanks!

vasily-kirichenko · 2017-03-14T20:15:53Z

I turned off committing on space in auto triggering mode, so even though it pops up in some inappropriate places, it's not annoying because you don't have to press Esc all the time to dismiss suggestions. I find it very usable now.

saul · 2017-03-14T20:20:04Z

vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs


    static member ProvideCompletionsAsyncAux(checker: FSharpChecker, sourceText: SourceText, caretPosition: int, options: FSharpProjectOptions, filePath: string, textVersionHash: int) = 
        asyncMaybe {
            let! parseResults, parsedInput, checkFileResults = checker.ParseAndCheckDocument(filePath, textVersionHash, sourceText.ToString(), options, allowStaleResults = true)

+            //Logging.Logging.logInfof "AST:\n%+A" parsedInput


Commented code

Yes. It will be very useful in further work on completion.

firewave-remo · 2017-03-15T06:07:38Z

In the moment we can use this in Visual Studio, you have recruited a new F# community member. I have never learned F# because it was so annoying when you have a C# Visual Studio Background and its also much easier to learn a new language with IntelliSense than without. So, thank you @vasily-kirichenko for your work!

vasily-kirichenko · 2017-03-15T07:20:49Z

@VSDekar Intellisense has worked since VS 2010, but it was not triggered automatically, except after dot. So saying that F# did not have intellisense is not fair.

firewave-remo · 2017-03-15T10:59:49Z

@vasily-kirichenko You are right, but thats why i said that when you come from C# and Visual Studio. The experience is (was :-) ) much better with C#.

…optimistically

smoothdeveloper · 2017-03-15T11:59:58Z

@VSDekar I think each language has it's advantage, but confinning the overal experience to intellisense (which is working in F# since day 1) is short sighted, I also experience better error prone code constructs in C# and a greater need for heavy tooling to perform simple refactorings 😄

So you have to factor the whole experience with language and then draw a conclusion, but stopping at intellisense doesn't look as fancy as in other language is too early.

KevinRansom · 2017-03-16T02:55:36Z

@VSDekar

Welcome to the team, we are very proud of the contributions of @vasily-kirichenko, @smoothdeveloper, @cloudRoutine and our other community members they are truly allowing our tooling to catch up with the C# and VB experience.

Kevin

smoothdeveloper · 2017-03-16T10:58:15Z

@KevinRansom I'm not contributing to VS much, I'd need some help with #2351 if possible to try to help with IDE features at some point.

* port Roslyn's C# completion triggering logic as is * fixed: completion do not trigger at all * fix position * fix a test * do not provide completion on certain places * fix muting completion on Named(Wild) pat in binding * do not provide completion on ctor / lambda parameter names + tests * fix binding traversing in completion * fix tests * fix the last failing test * "trigger completion after a character is typed" setting in a new completion property page * force intellisense property page loading * do not commit completion on space in auto triggering mode * fix completion list ordering

pqnet · 2017-08-30T10:05:40Z

The current (vs2017) trigger conditions are shit. Autocompletion triggers in places where syntax expects a new identifier (e.g. variable/type/member name/type parameter declaration) making it impossible to write a new identifier without pressing "esc" to close autocompletion. Try to declare a type parameter, such as in let myfun<'T> = ... without the 'T being autocompleted to 'tan

pqnet · 2017-08-30T10:08:30Z

There is no reason for type name completion anywhere else than after a colon (: ) or inherit keyword. C# rules are a bad template for F# because while declarations in C# start with a type, declarations in F# start with name (and and optional type after colon)

dsyme · 2017-08-30T11:23:48Z

Autocompletion triggers in places where syntax expects a new identifier (e.g. variable/type/member name/type parameter declaration) making it impossible to write a new identifier without pressing "esc" to close autocompletion.

@pqnet Could you please open a new issue for that, with as many examples as you can? thanks

cartermp · 2017-08-30T13:39:23Z

Also @pqnet ensure you're on the latest VS (15.3.3) when you test, as there are changes there to not commit autocompletion except for a small number of cases, fixing some annoyances in this space.

In general please do be respectful here; autocompletion is difficult stuff and represents a lot of time and energy from @vasily-kirichenko and others.

abelbraaksma · 2017-08-30T20:06:32Z

Autocompletion triggers in places where syntax expects a new identifier (e.g. variable/type/member name/type parameter declaration) making it impossible to write a new identifier without pressing "esc" to close autocompletion.

@pqnet Could you please open a new issue for that, with as many examples as you can? thanks

@dsyme, @vasily-kirichenko The same holds for trying to type an escaped identifier with `` (double backtick) syntax. I've reported this earlier here: #3406, but now you can't even type a single backtick-identifier that way without it will try to optimistically auto-complete after each space, comma, dot etc. The workaround is to constantly hit Esc. I'll update that issue accordingly.

Don't get me wrong, in any other code it usually works wonderfully and saves a lot of typing! But since 99% of my tests are written with this syntax, it makes writing them rather painful at the moment.

pqnet · 2017-09-12T15:00:56Z

@cartermp thank you, I was actually on 15.2 (didn't see any notification that a new version was available - I thought some would appear if my version were to be old) so probably my observation is obsolete. I'm sorry if my choice of words was a bit aggressive, I was frustrated and ended up being impolite. In my defense I tried to keep the criticism constructive and to give my contribution to the discussion: a concrete test case and my opinion about autocompletion triggering rules (there is no reason for it to trigger after every word, in particular after tokens which introduce a new name, like let, for, <, and similar). @dsyme I'll check whether these issues or similar ones can be reproduced in the new version or not, and eventually file another issue ticket for that, as you suggested.

CyrusNajmabadi · 2017-09-12T21:17:15Z

The way C# handles this is with 'builders'.i.e. try writing:

Func<int> f = s

We will pop up completion because you may want to refer to something with 's' in it. However completion will be in 'builder' mode. In this mode we do not commit by default. Instead the user has to explicitly pick an item and commit it themselves. The builder message also says that we're in a special mode because you may be about to write a lambda.

Basically the rules are:
If this is a reference-only location then open completion in normal mode.
If this is a location where you are only declaring a new variable name, do not pop up completion.
If it is a location where both could be happening (like above) them year the builder.

* port Roslyn's C# completion triggering logic as is * fixed: completion do not trigger at all * fix position * fix a test * do not provide completion on certain places * fix muting completion on Named(Wild) pat in binding * do not provide completion on ctor / lambda parameter names + tests * fix binding traversing in completion * fix tests * fix the last failing test * "trigger completion after a character is typed" setting in a new completion property page * force intellisense property page loading * do not commit completion on space in auto triggering mode * fix completion list ordering

vasily-kirichenko added 3 commits March 12, 2017 12:40

port Roslyn's C# completion triggering logic as is

d4dae88

fixed: completion do not trigger at all

a1f8ddf

fix position

aed1ceb

msftclas added the cla-already-signed label Mar 12, 2017

vasily-kirichenko mentioned this pull request Mar 12, 2017

Intellisense auto popup #406

Closed

fix a test

a949fc5

KevinRansom approved these changes Mar 12, 2017

View reviewed changes

vasily-kirichenko changed the title ~~Trigger completion optimistically~~ [WIP] Trigger completion optimistically Mar 13, 2017

Vasily Kirichenko added 3 commits March 13, 2017 11:31

Merge remote-tracking branch 'origin/master' into trigger-completion-…

1de44bc

…optimistically Conflicts: vsintegration/src/FSharp.Editor/Completion/CompletionProvider.fs

Merge remote-tracking branch 'origin/master' into trigger-completion-…

5f7c083

…optimistically

do not provide completion on certain places

89313c5

Merge remote-tracking branch 'origin/master' into trigger-completion-…

223f7ef

…optimistically

force intellisense property page loading

4c847b4

vasily-kirichenko changed the title ~~[WIP] Trigger completion optimistically~~ Trigger completion optimistically Mar 14, 2017

do not commit completion on space in auto triggering mode

2111be7

saul reviewed Mar 14, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into trigger-completion-…

c8949c9

…optimistically

fix completion list ordering

0a78887

vasily-kirichenko mentioned this pull request Mar 15, 2017

Intellisense list is sorted very wrongly #2618

Closed

KevinRansom approved these changes Mar 16, 2017

View reviewed changes

KevinRansom merged commit 4fd3f3e into dotnet:master Mar 16, 2017

vasily-kirichenko mentioned this pull request Mar 16, 2017

Fix completion performance #2621

Merged

WsbPro mentioned this pull request Mar 19, 2017

IntelliSense not working for variables (types etc) auto-completion #2659

Closed

CyberQin mentioned this pull request Apr 8, 2017

Same namespace cross project(C#,F#) will cause error. #2822

Closed

Trigger completion optimistically #2594

Trigger completion optimistically #2594

Conversation

vasily-kirichenko commented Mar 12, 2017

saul commented Mar 12, 2017

KevinRansom left a comment

Choose a reason for hiding this comment

majocha commented Mar 12, 2017

vasily-kirichenko commented Mar 12, 2017

majocha commented Mar 12, 2017

cartermp commented Mar 12, 2017

vasily-kirichenko commented Mar 13, 2017

cartermp commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

majocha commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

cartermp commented Mar 13, 2017

nosami commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

vasily-kirichenko commented Mar 13, 2017

nosami commented Mar 13, 2017

vasily-kirichenko commented Mar 14, 2017

smoothdeveloper commented Mar 14, 2017

vasily-kirichenko commented Mar 14, 2017

saul Mar 14, 2017

Choose a reason for hiding this comment

vasily-kirichenko Mar 14, 2017

Choose a reason for hiding this comment

firewave-remo commented Mar 15, 2017

vasily-kirichenko commented Mar 15, 2017

firewave-remo commented Mar 15, 2017

smoothdeveloper commented Mar 15, 2017

KevinRansom commented Mar 16, 2017

smoothdeveloper commented Mar 16, 2017

pqnet commented Aug 30, 2017

pqnet commented Aug 30, 2017

dsyme commented Aug 30, 2017

cartermp commented Aug 30, 2017

abelbraaksma commented Aug 30, 2017

pqnet commented Sep 12, 2017

CyrusNajmabadi commented Sep 12, 2017