Remove FCS syntax tree checks from typing assists #702
Conversation
DedSec256
force-pushed
the
ber.a/rewriteAssists
branch
3 times, most recently
from
7fa91e9 to
61f5e39
Compare
DedSec256
force-pushed
the
ber.a/rewriteAssists
branch
from
c4f1dfa to
96d60f8
Compare
DedSec256
force-pushed
the
ber.a/rewriteAssists
branch
from
96d60f8 to
3ac9a40
Compare
DedSec256
force-pushed
the
ber.a/rewriteAssists
branch
2 times, most recently
from
3fa45e1 to
07d1c49
Compare
DedSec256
changed the title
|
@DedSec256 Is is possible to extract and merge some manageable parts of this PR? I'd like to work on the typing assists, but don't want to add conflicts or more code to rework later. |
DedSec256
force-pushed
the
ber.a/rewriteAssists
branch
from
07d1c49 to
7e20444
Compare
|
|
||
| | _ -> defaultTraverse expr } | ||
| let range = | ||
| let inline checkExpr (expr: IFSharpExpression) (node: ITreeNode) = |
There was a problem hiding this comment.
What does checkExpr mean? Could then name convey what the function does actually?
There was a problem hiding this comment.
While I was coming up with a name for the function, I found an old bug for the case
for a = 0
to 10 do {caret}which resulted in
for a = 0
to 10 do
{caret}instead of
for a = 0
to 10 do
{caret}its' because here
the range was taken not from to ... expr, but do ..., which could never be matched with prevTokenEnd.
Fixed, test added.
ReSharper.FSharp/src/FSharp/FSharp.Psi.Features/src/FSharpTypingAssist.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Features/src/FSharpTypingAssist.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Features/src/FSharpTypingAssist.fs
Outdated
Show resolved
Hide resolved
| getEndOffset document lastRange = prevTokenEnd -> | ||
| Some range | ||
| let indentOffset = | ||
| let inline tryGetIndentOffset (exprBeforeKeyword: IFSharpExpression) (indentAnchor: ITreeNode) = |
There was a problem hiding this comment.
What is the reason to inline it?
There was a problem hiding this comment.
In order not to allocate a closure for tokenBeforeKeywordEnd, used inside
There was a problem hiding this comment.
Does it make any measurable difference?
Compared to reparsing the whole file after the Enter press, converting the resulting FCS tree into a R# one, type checking it, running the daemon, and calculating code completions, how much do we save here? I'd rather keep things simple and use the language default behavior, unless there's a convincing reason (e.g. we're going to allocate it hundreds/thousands times).
There was a problem hiding this comment.
And I also don't see where and how exactly it's allocated. Could you explain this, please? Thank you 🙂
There was a problem hiding this comment.
It could be viewed in Release mode in IL, since it has tokenBeforeKeywordEnd in closure
In any case, if this is an obstacle on the way to merge, I will remove inline, this is not so critical.
There was a problem hiding this comment.
Wow, I'd not expect a closure there at all. Thanks for showing this!
There was a problem hiding this comment.
Maybe it's worth trying to find a minimal repro and reporting it to the compiler team. Or is it expected and I'm missing something?
ReSharper.FSharp/src/FSharp/FSharp.Psi.Features/src/FSharpTypingAssist.fs
Outdated
Show resolved
Hide resolved
No description provided.