Unnecessary parameter info for commas and opening parenthesis within strings

[NoelAbrahams]

Hi,

VS: 2013 Update 4 RC TS: 1.3

Also occurs with ( and <

[johnnyreilly]

Complete aside @NoelAbrahams but what did you use to record and create the animated GIF? This could be useful to me!

Like the Throught for the Day BTW - I expect to hear you on Radio 4 soon. :smile:

[NoelAbrahams]

@johnnyreilly, I think all credit to @heroboy for introducing this. See #895

[johnnyreilly]

Oooh that's amazing! I'm off to download now....

[DanielRosenwasser]

I've actually been discussing this with @JsonFreeman because typing those characters in tagged template strings also trigger this sort of behavior. I think the desired behavior can get a little complex, but it's doable.

[DanielRosenwasser]

@CyrusNajmabadi and @mhegazy, is there any way that we could introduce the concept of"hard", "soft", and "continuation" triggers?

Specifically:

• A "hard" trigger means that the user specifically requested signature help (Ctrl+Shift+Space).
• A "soft" trigger means that we are hinting to the language service to provide signature help (These would be the keys for (, ,, ``,}`).
• A "continuation" trigger means that we were already showing signature help. A continuation trigger has higher priority than a "soft" trigger.

[JsonFreeman]

We already have two kinds of triggers (called Trigger and ReTrigger). I think what you are asking for is for the signature help query to also provide the trigger reason. In that case, if the language service encounters a comma inside a string literal, it would only show signature help on a ReTrigger, not a Trigger.

[mhegazy]

Looking at this again, i do not think the complexity of any solution here matches the value. i would be open to new suggestions to get this done without having to significantly change the interface between the host and the language service.

[DanielRosenwasser]

I don't think this should be marked as by design.

[mhegazy]

Do you have a proposal?

[JsonFreeman]

I think what should happen is the managed side should ask the script side if the comma is a trigger character. The script side can figure out that this is not a trigger character.

[mhegazy]

closing again as by design. please reactivate with a proposal if there is one, and outline what is the work needed.

[JsonFreeman]

I think the proposal I had in comment https://github.com/Microsoft/TypeScript/issues/1150#issuecomment-123066470 would work fine, it's just a matter of whether it's worth the extra plumbing.

[mhegazy]

The issue though annoying, is not common; you have to be in a string literal, that has a comma, while in a parameter help session. Moreover, a fix would require changes to the managed VS plugin, which is not an OSS component; so i can not just say "Accepting PRs", as that would be misleading. So options, either wait until the VS plugin is open sourced, and have some one sponsor the change, and fix it on both sides of the API, or some one on the core team decides to give it a try. if it is the latter, please feel free to reactivate when you have a fix. Either ways, I would like to see a skeleton of the implementation first, just to understand the scope (though i do not think it will be that big) as well as the perf impact if any.

[JsonFreeman]

Yes, makes sense. It's true that it would require changes on both sides, and the bug is only a minor annoyance (it doesn't block any scenario).

[CyrusNajmabadi]

I'm reopening this because it's pretty clearly a bug, and should not be considered "By Design". I agree that it will take work to fix it across several modules. But that's no reason to close something prematurely. Even if it takes us a while to get around to this, we should be tracking things properly and we should keep this in our system.

I would only close this in two situations:

1. We actually do not think this is an issue. I don't think this counts here. While minor, it is annoying and really shouldn't be happening.
2. We have a serious expectation that we would never fix this. I also don't think that's the case here. We've certainly fixed the same issue before for other languages.

There is no harm in accurately tracking problems in our products. We should not be 'losing' information by proactively closing bugs like these.

[CyrusNajmabadi]

The issue though annoying, is not common; you have to be in a string literal, that has a comma, while in a parameter help session.

True. But it does happen. I've certainly run into it before. And just because something isn't common doesn't mean that the bug should just be closed. The bug accurately reflects the state of the product, and provides a way for us to track this defect all the way to a good resolution (in some timeframe).

Moreover, a fix would require changes to the managed VS plugin, which is not an OSS component; so i can not just say "Accepting PRs", as that would be misleading. So options, either wait until the VS plugin is open sourced, and have some one sponsor the change, and fix it on both sides of the API, or some one on the core team decides to give it a try.

I think the latter is acceptable. Someone on the team can eventually fix it if they want. Or, we just wait until the managed side is open sourced as well (which I thought was happening soon thanks to @paulvanbrenk work). If we're already on the path to making this fully OSS, then it seems very premature to close this now.

Either ways, I would like to see a skeleton of the implementation first, just to understand the scope (though i do not think it will be that big) as well as the perf impact if any.

The perf impact is an interesting question. This goes in line with having the textual language service available for asking really quick synchronous questions. If/when we refactor the lexical classification service to serve more of that role, that would be a natural time to do this work as well.

I think it would be a good idea to maybe wait until the managed side is open sourced as well. These changes would serve as good PRs to help inform the community on how to make changes affecting both sides of things.

[paulvanbrenk]

@zhengbli do you know if this repros in the sublime plugin?

[JsonFreeman]

Good points, @CyrusNajmabadi. I have one question about this though:

This goes in line with having the textual language service available for asking really quick synchronous questions.

Wouldn't the language service have to have access the syntax tree to answer the question accurately about whether ',' is a trigger? It has to check if its an actual token, and whether its parent is the call expression.

[CyrusNajmabadi]

@JsonFreeman You are correct that the LS would need the tree to answer this question with total accuracy. The open question to me is if total accuracy would be necessary. The concern here is that this request is synchronous from the plugin side. And i'm loathe to perform tree operations synchronously :)

That said, it's quite possible this does not need to be synchronous. The entirety of the signature help MVC could be made asynchronous. Unlike completion (which needs to commit and block synchronously), sighelp has no such requirement.

So, if we made the API async, I would say that we should call over to the non-blocking syntactic thread to do the work. Otherwise, I would use the textual thread and would perform a 'good enough' check to catch 99% of cases.

Does that make sense?

[JsonFreeman]

Yup, both of those options make sense to me. My gut says to go with the async approach, but maybe that's just me being a perfectionist. I guess it largely depends on what heuristic is chosen for the synchronous approach. Maybe something like scanning that particular line of text to see if the comma actually gets scanned as a token (instead of part of a string or template literal or comment).

Actually rereading your post, I don't see a good reason to make this operation synchronous.

[zhengbli]

@paulvanbrenk no it doesn't repro. I think the decision of when to show the tooltip is a editor-specific thing.

[CyrusNajmabadi]

Going back to this:

The entirety of the signature help MVC could be made asynchronous.

So looking at the managed side, this remains true. however. I'm a little wary of going down that path. The reason for this is that this will essentially mean that in the internal machinery of sighelp we'll always be kicking off this async chain of events on every keystroke (since we don't know if the character will be a trigger character or not).

That's a lot of churning that can happen that i'd like to avoid.

However, from looking at this, the solution seems much easier and would go like this:

1. TS would still return "true" for IsTriggerChracter for the comma character.
2. Roslyn then calls into TypeScript through "ISignatureHelpProvider.GetItemsAsync(Document, int, SignatureHelpTriggerInfo, CancellationToken);"
3. That SignatureHelpTriggerInfo has a TriggerReason of "TypeCharCommand".

Because of this, you now know you were triggered because something was typed. In that case, you're now on the background, and you're async. You can just check then "was this trigger character really something we needed to trigger for". You'll send that over to the non-blocking syntactic thread, it will look at the tree, realize it's in a string, and say "no". You can then just return no items for GetItemsAsync.

This is much simpler, and fits into the model we have today.

[JsonFreeman]

Oh, good idea! The TriggerReason would actually contain the information that was missing before.

[DanielRosenwasser]

Yeah, I think that's what I originally meant by "soft"/"hard" trigger.

[JsonFreeman]

I have an idea to make this simpler. Instead of tracking a TriggerReason, only trigger signature help if you are not in some nested comma-separated list. This includes object literals, arrays, parameter lists, etc. Even if the user explicitly requested signature help! I am basing this on the assumption that the majority of signature help requests come from typed commas.

[marcoms]

This isn't limited to strings for me, this also happens within object and array literals after the comma - it makes calling functions that accept objects instead of repeated arguments very tedious. See #22106.

[DanielRosenwasser]

Hey guys what's the difference between a Trigger and a ReTrigger?

[DanielRosenwasser]

Here's my understanding:

• User-requested trigger: User hit ctrl+shift+space, specifically asked for signature help. The language service should always try to find signature help.
• Trigger: The editor was not previously showing signature help. The user hit ( or ,; the user presumably wants to get signature if available. The language service should respond if the trigger character appropriately corresponds to an argument list.
• ReTrigger: The editor is currently showing signature help, and the user hit ). The editor is now checking whether there is any outer signature help available at the cursor. The language service should always try to find a signature to keep the experience consistent.

Does that seem right? @amcasey @mjbvz @armanio123

[DanielRosenwasser]

The above seems to be validated. Here's what Roslyn calls them: https://github.com/dotnet/roslyn/blob/0472794308b0abbc6a9ab3bb30e0303410aae3df/src/Features/Core/Portable/SignatureHelp/SignatureHelpTriggerReason.cs

I'll add that from investigating, cursor movements cause retriggers.