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

Some errors are not reported if any of previous files contain errors #16708

Closed
auduchinok opened this issue Feb 14, 2024 · 5 comments · Fixed by #16719
Closed

Some errors are not reported if any of previous files contain errors #16708

auduchinok opened this issue Feb 14, 2024 · 5 comments · Fixed by #16719
Assignees
Labels
Area-Compiler-Service Various compiler service issues which do not belong to other labels/areas. Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Regression
Milestone

Comments

@auduchinok
Copy link
Member

Consider a project consisting of two files:

module AbstractBaseClass.File1

a
module AbstractBaseClass.File2

type AbstractBaseClass() =abstract P: int

There's an error expected for the type declaration:

Screenshot 2024-02-14 at 12 26 10

However, it's missing because of the undefined a error in the previous file. Commenting a out fixes the issue.

It seems it's a relatively recent regression, as I don't recall seeing this behavior previously.

@majocha
Copy link
Contributor

majocha commented Feb 14, 2024

If this is not Transparent Compiler, then I suspect this one #16576.

@vzarytovskii
Copy link
Member

If this is not Transparent Compiler, then I suspect this one #16576.

Yeah, looks likely, I can bisect it once back to normal internet

@0101
Copy link
Contributor

0101 commented Feb 16, 2024

Hmm, after investigating this for a bit, it seems to be on purpose...?

The diagnostics come from here:

conditionallySuppressErrorReporting (checkForErrors()) (fun () ->
try
implFileTypePriorToSig |> IterTyconsOfModuleOrNamespaceType (fun tycon ->
FinalTypeDefinitionChecksAtEndOfInferenceScope (cenv.infoReader, envAtEnd.NameEnv, cenv.tcSink, true, denvAtEnd, tycon))
with RecoverableException exn ->
errorRecovery exn m)

Explicitly having conditionallySuppressErrorReporting (checkForErrors()) where checkForErrors is A function to help us stop reporting cascading errors

let CheckOneImplFile
// checkForErrors: A function to help us stop reporting cascading errors

And we put diagnostics collected from previously checked files there.

let checkForErrors () =
(parseResults.ParseHadErrors || errHandler.ErrorCount > 0)

All of this seems to have been there for a while.

I am not sure what the purpose of this is:

// Play background errors and warnings for this file.
do
for err, severity in backgroundDiagnostics do
diagnosticSink (err, severity)

Or what "background diagnostics" are supposed to be. We put the previous files' errors there now - maybe that's just wrong?

@auduchinok
Copy link
Member Author

I think previously this would only affect the current file, i.e. it wouldn't affect showing errors in other files.

@0101
Copy link
Contributor

0101 commented Feb 19, 2024

Yeah that would make sense.

If I remove playing back background diagnostics, only this test fails:

// Set up a TestProject2 using an in-memory reference to TestProject but where TestProject has not
// compiled to be on-disk. In this case, we expect an error, because TestProject uses a generative
// type provider, and in-memory cross-references to projects using generative type providers are
// not yet supported.
begin
let testProjectNotCompiledSimulatedOutput = __SOURCE_DIRECTORY__ + @"/DUMMY/TestProject.dll"
let options = optionsTestProject2 testProjectNotCompiledSimulatedOutput
let fileName = __SOURCE_DIRECTORY__ + @"/data/TestProject2/TestProject2.fs"
let fileSource = FileSystem.OpenFileForReadShim(fileName).ReadAllText()
let fileParseResults, fileCheckAnswer = checker.ParseAndCheckFileInProject(fileName, 0, SourceText.ofString fileSource, options) |> Async.RunImmediate
let fileCheckResults =
match fileCheckAnswer with
| FSharpCheckFileAnswer.Succeeded(res) -> res
| res -> failwithf "Parsing did not finish... (%A)" res
printfn "Parse Diagnostics (TestProject2 without compiled TestProject): %A" fileParseResults.Diagnostics
printfn "Check Diagnostics (TestProject2 without compiled TestProject): %A" fileCheckResults.Diagnostics
fileCheckResults.Diagnostics
|> Array.exists (fun diag ->
diag.Severity = FSharpDiagnosticSeverity.Error &&
diag.Message.Contains("TestProject.dll"))
|> shouldEqual true
end

#16719

Interestingly Transparent Compiler doesn't have this problem. Never mind, it does if the files depend on each other, which just wasn't the case in the example.

@0101 0101 added Area-Compiler-Service Various compiler service issues which do not belong to other labels/areas. and removed Needs-Triage labels Feb 19, 2024
@0101 0101 self-assigned this Feb 19, 2024
@0101 0101 added Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Regression labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-Service Various compiler service issues which do not belong to other labels/areas. Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Regression
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants