qsharp-compiler icon indicating copy to clipboard operation
qsharp-compiler copied to clipboard

Fix several cases of incorrect diagnostic ranges for call expressions

Open bamarsha opened this issue 2 years ago • 2 comments

  • When inferring a call expression in a function (ExpressionVerification.fs:885-886), the constructed functionType can end up as the representative for the call expression's type, if the expected callable type is a variable. That means it can be used for diagnostic ranges in certain cases, so it should have a non-generated range.
  • In InferenceContext.Relate, binding a type variable can cause type constraints to be checked for types that aren't descendants of the current expected and actual types. That means that Diagnostic.withParents shouldn't be called on the diagnostics returned by tryBind.
  • Diagnostic.withParents should default to the current types in the case of an orphan context for the purposes of checking if the ranges changed - otherwise, the Option.forall assumed the range did not change for an empty option.

This fixes the range of diagnostics in cases like these:

Controlled X([1], 2);
             ~~~  ~
let f = op -> Adjoint op();
                      ~~
let g = op => op();
g(X);
  ~

Closes #1366.

bamarsha avatar Apr 14 '22 01:04 bamarsha

In a meeting it was mentioned that it would be nice to have tests for these changes. This PR does not change which diagnostics are generated, only the range and message of each diagnostic, both of which are properties that are not checked by any of the existing diagnostic tests.

I can look into adding test utilities for checking these properties so that at least new tests could optionally take advantage of them. I know @bettinaheim has been aware of this test gap and might have some thoughts also.

bamarsha avatar Apr 18 '22 21:04 bamarsha

In a meeting it was mentioned that it would be nice to have tests for these changes. This PR does not change which diagnostics are generated, only the range and message of each diagnostic, both of which are properties that are not checked by any of the existing diagnostic tests.

I can look into adding test utilities for checking these properties so that at least new tests could optionally take advantage of them. I know @bettinaheim has been aware of this test gap and might have some thoughts also.

I think it would be good to set up a test validating diagnostic ranges starting with just testing the case at hand. We won't close the test gap, but diagnostic ranges seem fragile (one of the more frequent sources of bugs), so let's take the first step. @samarsha Do you mind setting up some compiler tests for validating diagnostic ranges? You can focus on the case at hand and we will add to them over time. Let's also scope it to validating the compiler rather than any IDE feedback.

bettinaheim avatar Jun 15 '22 21:06 bettinaheim

Do you mind setting up some compiler tests for validating diagnostic ranges? You can focus on the case at hand and we will add to them over time. Let's also scope it to validating the compiler rather than any IDE feedback.

Added.

bamarsha avatar Sep 08 '22 00:09 bamarsha