Unused declarations analyzer

[vasily-kirichenko]

[vasily-kirichenko]

Code fix that works on the compiler's FS1182 should work when VS fixes F# code fixes.

[cartermp]

Good stuff.

[vasily-kirichenko]

It does not work as expected for two reasons:

• We need full project check results, but calling ParseAndCheckProject blocks Reactor for long time, which we should never do
• ParseAndCheckProject always reads all files from disk (via FileSystem), it means that the analyzer does not work on dirty buffers, even on a single one (that's the reason our ProjectDiagnosticAnalyzer is commented out)

@dsyme Am I right on these points? To fix the first one we need something like ParseAndCheckProjectInBackground: FSharpOptions -> Async<FSharpProjectCheckResults>, which starts background compilation, then waits asynchronously until it finishes. I guess we don't have such function yet.

[vasily-kirichenko]

And the third point

• It crashes VS :)

[saul]

I've seen this is quite a few places - is there nowhere we can have something like:

module ImmutableArray =
    let ofSeq (x : 'a seq) = x.ToImmutableArray ()

That way we can do:

unusedSymbolUses
|> Seq.map (* ... *)
|> ImmutableArray.ofSeq

Just a thought

[vasily-kirichenko]

Yeah, good suggestion. Or we could provide an implicit conversion from seq<'a>... ;)

[dsyme]

We need full project check results

There is already an implicit background project check happening (that's the IncrementalBuilder). It currently raises events when finished. So you could listen for the ProjectChecked event and then call ParseAndCheckProject and GetBackgroundCheckResultsForFileInProject, which should in that case return more or less immediately (if nothing goes dirty in between the calls). Or we could change the ProjectChecked event to return the relevant FSharpCheckProjectResults.

ParseAndCheckProject always reads all files from disk (via FileSystem),

Yes, the background IncrementalBuilder reads via FileSystem. Using dirty buffers for (intellisense and errors too) will be interesting - it will change the feel of the tooling but is clearly the "right" thing.

Why does the analysis need full project results? In the screen shot you're only doing locals. Do you grey out private class/method/function declarations too? But in that case wouldn't you need the whole solution graph to properly treat internals w.r.t. InternalsVisibleTo?

[vasily-kirichenko]

So you could listen for the ProjectChecked event and then call ParseAndCheckProject and GetBackgroundCheckResultsForFileInProject, which should in that case return more or less immediately (if nothing goes dirty in between the calls). Or we could change the ProjectChecked event to return the relevant FSharpCheckProjectResults.

The problem is that we cannot tell Roslyn "hey, we have fresh data for you". It calls us itself when a file changed. Or you suggest to do something like:

let compilationToken = StartBackgroundCompilation(options)
let! eargs = Async.AwaitEvent ProjectChecked
if eargs.CompilationToken = compilationToken then ...
else <wait again>

?
I'm trying to say that we should be sure that background compilation results are not outdated.

[vasily-kirichenko]

Do you grey out private class/method/function declarations too?

Yes.

But in that case wouldn't you need the whole solution graph to properly treat internals w.r.t. InternalsVisibleTo?

Yes, but I ignore InternalsVisibleTo because full solution type checking is not possible on real worlds solutions. Even if we moved FSC to an external process, checking would take minutes.

[vasily-kirichenko]

Yes, the background IncrementalBuilder reads via FileSystem. Using dirty buffers for (intellisense and errors too) will be interesting - it will change the feel of the tooling but is clearly the "right" thing.

We should finally implement #1866 (comment)

[dsyme]

The problem is that we cannot tell Roslyn "hey, we have fresh data for you". It calls us itself when a file changed. Or you suggest to do something like...

Yes that's approximately right. However it's dodgy, e.g. the event is not guaranteed to be raised. The background builder just builds one project (and its dependencies), selected somewhat implicitly (see ImplicitlyStartCheckProjectInBackground). If the user switches to a different project the event will never be triggered, or may be triggered at a random arbitrary later point.

Suggest:

1. turn off the use of implicitlyStartBackgroundWork , and
2. call StartBackgroundCompile explicitly when the user switches to a file in the project, and
3. modify StartBackgroundCompile to return an Async<FSharpCheckProjectResults>, and
4. extend FSharpCheckProjectResults to also contain the table of FSharpParseFileResults and FSharpCheckFileResults, and
5. make sure only one StartBackgroundCompile is active, cancelling all others.
6. when/if a StartBackgroundCompile completes, apply any and all formatting that depends on the overall project

The advantage of all this is that it gives me more confidence that we won't be putting any extra strain on the reactor thread - it's just doing what we're doing already, we just happen to be listening in for the completion of checking a project.

[vasily-kirichenko]

@dsyme good plan. Unfortunately, RC3 crashes in 5-30 seconds after start both at home and at work. So I'm gonna wait until RTM is released to continue working on VFT.
dotnet/roslyn#16900
(I'm gonna reinstall windows at home to be sure it's not my setup, of course, but I doubt it'll help)

[vasily-kirichenko]

Reset windows, installed RC3. Everything works OK so far :)

[KevinRansom]

@vasily-kirichenko
Is this complete? if so please resolve conflicts and strip [WIP] from the title.

Thanks

Kevin

[vasily-kirichenko]

@KevinRansom It requires #1866 to be done (checking project wide against dirty buffers).

[KevinRansom]

@vasily-kirichenko @dsyme

there seems to be some difference of opinion about how to proceed with #1866. Is #1866 a pre-requisite for this PR, or would it just make it work better.

Kevin

[dsyme]

@KevinRansom I agree it requires #1866 to be done

[KevinRansom]

@dsyme Is the current #1866 is the right approach, or will it need an alternative implementation?

[dsyme]

@KevinRansom I'll defer to @vasily-kirichenko - he implemented this for VFPT. For VFT it woud also apply to error lists, intellisense and quick info - we'd need to trial it extensively

[KevinRansom]

@vasily-kirichenko , is this ready or do you want to do more work?

Kevin

[vasily-kirichenko]

It still requires #1866 to be implemented first.

[vasily-kirichenko]

I will simplify it to analyze private for document symbols 'coz no change project wide analysis will every be implemented.

[KevinRansom]

@vasily-kirichenko is this ready to merge?

Thanks

Kevin

[KevinRansom]

@vasily-kirichenko please let me know when this is ready to merge?

[vasily-kirichenko]

@KevinRansom it has false positives (it detects as unused inherit BaseType() for instance). So I need to investigate it.

[KevinRansom]

@vasily-kirichenko sounds good, change it from wip, whan it's ready for review. And thanks for this.

Kevin

[vasily-kirichenko]

As we cannot get project check result based on dirty documents, I cut this analyzer to check private symbols only (i.e. ones scoped to file):

It's super fast and should work reliably.

It's ready to merge.

[KevinRansom]

Thank you for this work