[analyzer_testing] Make rule tests (`lint` method) use `TestCode` ranges

[FMorschel]

Similarly to #60234, I've seen the lint rule tests and similar be offset on Windows.

I propose we deprecate the existing lint and error methods provided by analyzer_testing to prefer using a version that uses the ranges from TestCode similarly to assist tests.

We could still accept the other parameters but we would accept the range index instead of the offset and length. This would also help with the normalisation work done at the before-mentioned issue.

CC @RohitSaily @bwilkerson @DanTup @srawlins

[RohitSaily]

In addition to what you mentioned, I think it's also a good idea to make things more consistent. I also find it more natural to specify a range

It would be nice if there were another way to specify the location of a lint -- eg describe the code that is being linted somehow -- but I suppose anything else would involve some sort of searching and matching and be inefficient across a lot of tests and is probably not worth the effort to introduce in the SDK. So I'm on board with using ranges

[FMorschel]

Yes, in refactoring tests we were using text matching (then working with the offset) but that can be both innefficient and error prone because if you accidentally write text that is not unique, it can be wrong.

We are moving refactor tests to use TestCode:

void f() {
  var /*[0*/variableName/*0]*/ = 0;
  print(variableName);
}

Means the whole range that is declaring the variable name.

We parse the code first and remove the markers and annotate the values to work with them later.

[srawlins]

@FMorschel I am unfamiliar with TestCode. Can you provide some examples and/or links, and also some motivation :D. I don't know of any downsides of using error or lint and Windows.

[DanTup]

@srawlins TestCode is the class we use for parsing markers like [!foo!] in test code:

sdk/pkg/analyzer/lib/src/test_utilities/test_code_format.dart

Lines 10 to 40 in 7dd8aed

	/// A class for parsing and representing test code that contains special markup
	/// to simplify marking up positions and ranges in test code.
	///
	/// Positions and ranges are marked with brackets inside block comments:
	///
	/// ```
	/// position ::= '/*' integer '*/'
	/// rangeStart ::= '/*[' integer '*/'
	/// rangeEnd ::= '/*' integer ']*/'
	/// ```
	///
	/// Numbers start at 0 and positions and range starts must be consecutive.
	/// The same numbers can be used to represent both positions and ranges.
	///
	/// For convenience, a single position can also be marked with a `^` (which
	/// behaves the same as `/*0*/`). A single range can be marked with `[!` and
	/// `!]`, which behave the same as `/*[0*/` and `/*0]*/`.
	///
	/// In addition, the pattern `/**/` will be removed from the test code. This is
	/// be used for two purposes.
	///
	/// First, it can prevent certain code from being interpreted as markup. For
	/// example, if the test code includes `[!` (such as in a list literal whose
	/// first element begins with a unary prefix operator), you can use `[/**/!` to
	/// prevent the `[!` from being interpreted as markup. Similarly, you can use
	/// `!/**/]` in places where `!]` should appear in the unmarked code.
	///
	/// Second, it can be used at the end of a line of code that contains trailing
	/// whitespace. Without some form of marker the formatter will remove the
	/// trailing whitespace.
	class TestCode {

Previously we had a mix of different ways of annotating test code to mark regions/positions, and this class was created to try and standardise them.

The issue @FMorschel is refering to with windows is that hard-coding numeric offsets can break on Windows if we normalize the line endings. For example:

var content = '''
one
two
''';

Here, the offset of two will depend on the newline character being used. The SDK has gitattributes to always check out with \n so it would be consistent, however in most places we're now normalising the line endings for code files created in tests for the platform, to ensure we are testing that we correctly handle \r\n on Windows (there have been bugs in the past where we would produce edits with just \n regardless of the actual file line endings).

So instead, we can write:

var code = TestCode.parse('''
one
[!two!]
''');

And code.range will give us the range marked with [! !], and code.code gives the code with those markers removed. We support ranges and positions, with shorthand ([!/!] and ^ for single range/positions, or using numbers otherwise).

(the docs on TestCode explain this in more detail)

[FMorschel]

Exactly! This is why @RohitSaily's CL for #61885 had failing bots. The offsets were different. So I'm suggesting we go with the TestCode approach for parsing the ranges in markers. This way no matter the line endings we still get the correct offsets.

We already do that for many other tests like renames, occurences, folding, and probably others I can't think of right now.

Currently assist tests and many others come from the same base class that already handles the defining the test code to a TestCode and everything else. We have a mixin for setting the different offsets of ranges or positions (this defines the offset and length).

So making the change for the rule tests, we can make some progress in making this class work for other tests and unifying things for the other issue I've opened on exposing API to test assists/fixes.

[srawlins]

Thanks @DanTup and @FMorschel. But how does this come into play with tests that assert where diagnostics are produced? Like, we're not producing edits in these tests.

This is why @RohitSaily's CL for #61885 had failing bots. The offsets were different.

How is that possible with the .gitattributes that Danny mentioned? (*.dart text eol=lf)

[DanTup]

But how does this come into play with tests that assert where diagnostics are produced?

In the tests at https://dart-review.googlesource.com/c/sdk/+/459020 there is [lint(53, 18)], so this could be replaced by something like [lint(code.range)], or in the case of multiple diagnostics, something like [lint(code.ranges[0]), error(code.ranges[1])].

Or you could annotate the ranges (by number) and then pass through a map with additional info, for example the folding range tests do this so they can verify the specific folding kinds:

sdk/pkg/analysis_server/test/lsp/folding_test.dart

Lines 250 to 254 in 7dd8aed

	expectRanges({
	0: FoldingRangeKind.Comment,
	1: FoldingRangeKind.Imports,
	2: FoldingRangeKind.Comment,
	});

How is that possible with the .gitattributes that Danny mentioned? (*.dart text eol=lf)

I'm not sure, but I think some of the global git settings (like autocrlf?) might override this - so depending on your global git settings, you might still get crlf (but the bots probably don't have autocrlf set). Perhaps this could be fixed by forcing this in the repo too, I'm not certain, would need some testing.

(But, even if we can make this repo consistent, I don't think that's the right fix - we want to be sure that we test both code with \r and \r\n to ensure we haven't got any code that makes assumptions about line endings that might behave incorrectly with real \r\ns in users code.

[FMorschel]

This also happened to me before. When running the tests locally, I never see them pass. I must always remove the amount of eols before the line in question. Because while I've not yet committed, the offsets are wrong.

So in this case, the change I'm proposing only affects Windows users while contributing and with test normalisation so all our tests with ranges look the same.

And for the methods I'm proposing to change, we'd obviously need to add the range marks to our test code and then instead of giving the offset and length to the method, we'd only need to give the index of the range in question (or if we want to leave the methods as they are, we could get the range and pass in the offset and length by the CodeRange - I think that is the class name - they produce).

[srawlins]

Because while I've not yet committed, the offsets are wrong.

Ah that's very interesting. Yeah that would be super annoying 😅

This makes sense to me.

[RohitSaily]

@srawlins

This is why @RohitSaily's CL for #61885 had failing bots. The offsets were different.

How is that possible with the .gitattributes that Danny mentioned? (*.dart text eol=lf)

In my case it wasn't the line endings but something with how my editor was formatting the indents in strings even after I would run dart format (which, I'm guessing, doesn't reformat string contents anyways)

[bwilkerson]

In my case it wasn't the line endings but something with how my editor was formatting the indents in strings ...

Formatters really shouldn't change the content of literal strings. I'm guessing that the one you're referring to doesn't understand the multi-line strings in Dart.

But I'll also note that if the formatter you're talking about is changing something that dart format would change back (something outside string literals), then it won't pass the pre-commit checks. You might want to see whether you can disable that formatter when working on SDK code.