Improve perf of unused opens analysis #4497
Conversation
|
I have manually checked this using F5 from VisualFSharp.sln and the performance is ok.
So this is ready once green (I need to make sure it continues to pass @vasily-kirichenko's original unussed-open tests) |
|
OK, this is ready |
|
@auduchinok Could you review please? |
| // extension members should be taken into account even though they have a prefix (as they do most of the time) | ||
| // Extension members should be taken into account even though they have a prefix (as they do most of the time) | ||
| true | ||
| | _ -> |
There was a problem hiding this comment.
Would it make sense to filter out more symbol uses here? All local values, parameters and type parameters could be filtered out before calling QuickParse methods and getting document line strings. Symbol bindings could also be filtered but there're several known cases where a symbol use is considered as binding even though it's from a usage.
There was a problem hiding this comment.
Yes, absolutely, good feedback , thanks
| let filterOpenStatements symbolUses openStatements = | ||
| async { | ||
| let! results = filterOpenStatementsIncremental symbolUses openStatements [] [] | ||
| return results |> List.map (fun os -> os.Range) |
There was a problem hiding this comment.
This comment isn't about the PR itself but is about ranges this feature deals with.
I'd in favour of having SynMemberDefn.Open tree nodes here (and other similar cases where a range is returned to a client) instead of lid ranges if these nodes had ranges of the whole expressions instead of its long identifiers. Furthermore, it'd be great if other declarations (namespace, type, etc) ranges included starting keywords (or even pointed to a token pos) so the range was the actual range of the declaration.
There was a problem hiding this comment.
That makes sense (but yes, is orthogonal)
There was a problem hiding this comment.
This #4106 is related. We should have discussions about that in there.
Also, SynMemberDefn.Open doesn't seem to be used anywhere that I can find in VisualFSharp.
There was a problem hiding this comment.
@TIHan we use it in Rider along with other types from AST.
There was a problem hiding this comment.
Correction, I don't see SymMemberDefn.Open being constructed in VisualFsharp.
There was a problem hiding this comment.
@TIHan it looks I copied the wrong type name in the first message. It should have been SynModuleDecl.Open, thank you.
|
OK, then this is ready to merge into master (and then will get merged into dev15.7 as normal) |
|
@TIHan can you give this a review? We definitely want to get this into the 15.7 branch and line it up for Preview 3 |
5 participants
Attempts to fix #4488, and documents the code a bit more. However this doesn't address the issue fsharp/fsharp-compiler-docs#830, which is "hard".
Things fixed:
Seq.cachewas faulty, since it was returned at the end of a property, meaning the cache was never actually effectiveThing improved:
I can't test this properly until #4496 is fixed.