Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ParseAndCheckProject no longers reports all diagnostics #15337

Closed
wants to merge 1 commit into from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jun 7, 2023

A fractured flaw, a broken hue, hath beset my project's sight,
Type checking, bereft of full diagnostics, relinquishes its might,
Where once clarity and guidance did resound,
Now echoes of disarray, an enigmatic sound.

Hi all,

This is more of a question and potentially a bug report disguised as a PR.
I believe I found a regression when it comes to the compiler diagnostics of implementation files that were backed by a signature file.

See MissingDiagnostic.fs for a concrete example.
The diagnostic reported in the implementation file is not covered anymore by FSharpChecker.ParseAndCheckProject.

I suspect this is happens in FinalizeTypeCheckTask because

let! tcInfos =
computedBoundModels
|> Seq.map (fun boundModel -> node { return! boundModel.GetOrComputeTcInfo() })
|> NodeCode.Sequential

will contain the same diagnostics that were reported in the signature file for both files.
The implementation file will wrap the signature diagnostics in

let skippedImplemetationTypeCheck =
match syntaxTreeOpt, prevTcInfo.sigNameOpt with
| Some syntaxTree, Some (_, qualifiedName) when syntaxTree.HasSignature ->
let input, _, fileName, _ = syntaxTree.Skip qualifiedName
SkippedImplFilePlaceholder(tcConfig, tcImports, tcGlobals, prevTcInfo.tcState, input)
|> Option.map (fun ((_, topAttribs, _, ccuSigForFile), tcState) ->
{
tcState = tcState
tcEnvAtEndOfFile = tcState.TcEnvFromImpls
moduleNamesDict = prevTcInfo.moduleNamesDict
latestCcuSigForFile = Some ccuSigForFile
tcDiagnosticsRev = prevTcInfo.tcDiagnosticsRev
topAttribs = Some topAttribs
tcDependencyFiles = fileName :: prevTcInfo.tcDependencyFiles
sigNameOpt = Some(fileName, qualifiedName)
})
| _ -> None
.

tcDiagnosticsRev = prevTcInfo.tcDiagnosticsRev being the culprit.

Later in FinalizeTypeCheckTask
let finalInfo = Array.last tcInfos and let diagnostics = diagnosticsLogger.GetDiagnostics() :: finalInfo.tcDiagnosticsRev will the wrong diagnostics.

This is breaking behaviour if you compare it against FCS 43.7.200.
See MissingDiagnostic.zip sample to compare them.

Note that changing FSharpChecker.Create(enablePartialTypeChecking = enablePartialTypeChecking) doesn't seem to have any effect on this.

The changes in #14903 might be related or relevant here.
@majocha would you mind taking a look at this?

@nojaf nojaf requested a review from a team as a code owner June 7, 2023 12:19
@nojaf nojaf changed the title Add unit tests to illustrate the problem. ParseAndCheckProject no longers reports all diagnostics Jun 7, 2023
@vzarytovskii
Copy link
Member

vzarytovskii commented Jun 7, 2023

Thanks, we've seen it happening for compiler too.

I wish GH had a mix of issue and PR, when you can submit and issue, but have some diff too, so CI can run tests, but it would still be in the issues view.

@majocha
Copy link
Contributor

majocha commented Jun 7, 2023

When I got the notification my first thought was "it would be awesome to have a test for this", then I opened Github and what a pleasant surprise 🤩. Great writeup @nojaf as usual.

I think I got it. This what I have now: (I'll post a draft later)
image

I'm not sure what the logic should be in case of enablePartialTypeChecking = true? Should GetCheckResultsAndImplementationsForProject do a full typecheck regardless?

@majocha
Copy link
Contributor

majocha commented Jun 7, 2023

Yeah, the problem is that collecting diagnostics sequentially does not really work well when we're not doing the type check sequentially. I understand this is solved nicely in #14494. Transparent Compiler PR #15179 will tackle this also. I'm not sure it's worth putting the effort here in the old code that is on it's way out already?

@vzarytovskii
Copy link
Member

Yeah, the problem is that collecting diagnostics sequentially does not really work well when we're not doing the type check sequentially. I understand this is solved nicely in #14494. Transparent Compiler PR #15179 will tackle this also. I'm not sure it's worth putting the effort here in the old code that is on it's way out already?

Problem with this is that new compiler will take a really long time until we can turn it on by default for vs.

@nojaf
Copy link
Contributor Author

nojaf commented Jun 8, 2023

I'm not sure it's worth putting the effort here in the old code that is on it's way out already?

Given this is quite a regression, I think it is worth putting effort into this. As this is a public API of FCS, it will impact many consumers.

I guess the implementation file (with backing signature) should only be allowed to be skipped for FSharpChecker.ParseAndCheckFileInProject. And never be skipped in ParseAndCheckProject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants