Detect AttributeApplication completion context for unfinished attributes #4126
Conversation
vasily-kirichenko
force-pushed
the
cut-off-attribute-suffix-in-completion
branch
from
2f58955 to
3673d66
Compare
| match untypedParseOpt with | ||
| | Some upi -> upi.ParseTree | ||
| | None -> None | ||
| let TryGetCompletionContext (pos, parsedInputOpt: ParsedInput option, lineStr: string) : CompletionContext option = |
There was a problem hiding this comment.
Can we go further and do parsedInput : ParsedInput instead of parstedInputOpt : ParsedInput option ?
|
@TIHan done. |
| @@ -1283,6 +1282,47 @@ module UntypedParseImpl = | |||
| } | |||
|
|
|||
| AstTraversal.Traverse(pos, pt, walker) | |||
| // Uncompleted attribute applications are not presented in the AST in any way. So, we have to parse source string. | |||
There was a problem hiding this comment.
My gut feeling tells me we should try to get something useful from the parse tree instead of trying to do some sort of ad-hoc parsing ourselves to determine what context we are in.
Consider this code:
[<
(* marker *)
>]
module Test =
let beef = 1Completions do not work here because we are checking lineStr. IMHO, we should avoid lineStr and parsing ourselves to determine what context we are in at all costs.
If we are getting nothing useful from the parse tree, then we need to modify the parser. That way we can handle most if not all cases. The implementation would be much simpler too.
Also, in the future, these big new lines code should probably go into a separate function rather than make an existing function bigger than what it was.
There was a problem hiding this comment.
@TIHan This also applies for current uses of QuickParse that uses a similar approach of getting a context for possibly unfinished expressions. Moving to AST there as well would be a great move.
There was a problem hiding this comment.
I'm very happy that we are starting to tackle this. Also the tests are fantastic.
However, I'm not exactly feeling good doing our own parsing we when have a perfectly good F# parser to tell us what context we are in. If the F# parser gives us nothing to work with, we need to modify it to support tooling. (See code comment)
|
Yes, I agree.
|
|
I'm glad we agree. I know this covers most cases, but this adds to maintenance cost to support this kind of code over time. I want to look at this myself, but I need to adjust some priorities. Recently, I've been trying to get the new project-system that .NET is using to have great F# support; that's been my number 1 priority. |
|
That’s sad because less and less people care about VS these days. |
|
Let’s stay on topic here
On Sat, Dec 16, 2017, at 12:10, Vasily Kirichenko wrote:
That’s sad because less and less people care about VS these days.
— You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub[1], or mute the
thread[2].>
|
|
@TIHan although I agree that adding this ad-hoc parsing isn't the best approach in the long-term, I think that for completion needs it covers most of the cases and @vasily-kirichenko has done a great job implementing it and thoughtfully covering it with tests. The right approach would be to implement it in the parser but until somebody does it, all editing tools could benefit by using this approach. The person who will implement it in the parser could be me, could be @TIHan, or someone else from the Visual F# team or the community. When all needed info is preserved in the AST we probably won't need to continue using this approach anymore so I hope that it's more a short-to-middle-term rather than a long-term approach. When switching to using AST, we'll be able to use all tests Vasily implemented so it helps even in the long-term to prevent regressions. I'm glad Vasily implemented it now this way so everyone gets a better completion before it is done the right way. |
|
@auduchinok , there is absolutely no doubt that @vasily-kirichenko has done a great job. I have the utmost respect for his work, not only here, but his contributions to the entirety of the tooling that we have. From my perspective, I think it is best we learn how to do these types of changes/fixes the right way if it doesn't require that much more work. Once we do, then it will be even easier to always to do the right thing and benefit everybody. |
|
@TIHan If you could spike something like this on the parser to assess difficulty, that would help where to take this PR. If it's really simple, we should just do that. But if it ends up being fraught with difficulty and/or weird problems, we should take this PR and think about how to do it via the parser. |
|
Either way, all of the tests here are worth taking no matter what happens. |
|
@cartermp That is fair. I agree the tests are worth taking for sure. I'm going to look at the parser and see what can be done. We just need to be patient and carefully think this through. |
|
After looking at the parser, it is definitely possible to make this happen when get better recovery for the attributes. I don't believe it is a scary task, but I do think it will take longer to understand. I still think we should pursue the right way to do this, but I'm leaving that up to everyone else. If everyone believes this is fine for now, then it's fine. Therefore, I'm approving. |
|
@saul As the algorithm is based on a current source line, I'm sure it works in your case. I'll add a test. |
|
I've refactored the tests, they don't look too scary anymore :) |
|
Perfect thanks @vasily-kirichenko |
|
@vasily-kirichenko is this done? If so, I will merge it. :) |
|
yes |
|
Thank you. This is great. I'm created an issue for the error recovery for attributes here. That's the first step in using parse tree to determine context. |
…tes (dotnet#4126) * detect AttributeApplication completion context for unfinished attributes * fix whitespace sensitiveness, add more tests * more tests * support ":" in attribute names fix tests * fix tests * pass ParsedInput instead of ParsedInput option to TryGetCompletionContext * refactor tests
No description provided.