feat: Introduce baseline system to suppress existing errors

[softius]

Summary

Declare currently reported errors as the "baseline", so that they are not being reported in subsequent runs. It allows developers to enable one or more rules as error and be notified when new ones show up.

Related Issues

• Change Request: Introduce a system to suppress existing errors

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: softius / name: Iacovos Constantinou (a99c456, c9a718c, 4d73750, 9e035f1, ed01be1, f9b3976)

[bradzacher]

the issue with using a count is that it's possible to change the file without changing the count - i.e. you add as many new reports as you remove.

EG in this example the code has meaningfully changed, but the total number of reports is unchanged. So the file would not violate the baseline.

/// before

/* eslint no-console: error */
export function test(condition) {
  if (condition) {
    console.log();
  } else {
    return 1;
  }
}

/// after

/* eslint no-console: error */
export function test(condition) {
  if (condition) {
    return 1;
  } else {
    console.log();
  }
}

personally I've preferred more granular checks - eg the line + col as it's stricter. It's more likely to false-positive, but it's less likely to false-negative (which is more valuable, IMO).

[ondrejmirtes]

Hey, PHPStan creator here (where the baseline is implemented for the past almost 5 years). It's the most popular feature by far (yeah, the most popular feature of a static analyser is ignoring errors).

Turns out that the count is the right amount of granularity. Expecting "having this error in this file this many times is okay" is really useful. If you included the line number in the baseline entry, it'd get obsolete really fast. It'd essentially be an inline ignore, but in an external file. Not really practical.

If someone manages to remove an error but add it in a different place in the same file, good for them. Having a baseline means you're ignoring some errors on purpose, and this situation most likely means it's the same error, it just moved.

PHPStan is a little bit more granular becauses the baseline includes exact error messages being ignored, but this proposal only includes error types. Maybe that's something to consider.

An alternative approach that preserves line numbers would be to also include the git commit hash from the moment the baseline was generated. So "this error on line 12 in commit 34afddac". This would allow shifting expected errors around based on git diff. So three commits later, if there was a new line added on line 5, the baseline would expect the error to occur on line 13.

But that can become computationally expensive as the diff between the original commit and HEAD grows bigger (if the baseline is not regenerated for a long time).

Anyway, I'm really excited other programming tools to adopt the baseline concept, it's really useful!

[softius]

From my point view, the baseline is an "agreement" between the developers and eslint that it is acceptable to have X number of errors, from a particular rule, for a particular file.

You are both right @ondrejmirtes @bradzacher , someone could fix one error and create another one or move it to a different line and this won't be reported. Personally, I don't see that as a problem. If that is necessary, maybe it can be resolved at the git / ci/cd level similarly to the suggestion provided by @bradzacher below.

[nzakas]

I think the error count is fine for a first pass at this feature.

[bradzacher]

the alternative we've introduced at Canva is that we designate specific rules as "in migration" and we only consider reports from those rules if they exist in changed files (according to git comparison against the main branch).

With this system developers must address lint errors if they touch a file but otherwise they can be ignored.

This does require integration with the relevant source control system - though we've found it works quite well.

[fasttime]

Thanks for the RFC @softius. Can you please sign the CLA as requested in this comment?

[ondrejmirtes]

In case of PHPStan, this flag optionally accepts a filename. In some cases people generate multiple different baselines. It allows you to have a different baseline from the points in time where you enabled different rules.

It'd also mean that these baseline files would have to be referenced from the main eslint config because they couldn't be auto-discovered anymore. That's a potential downside.

[softius]

Yes, exactly. I was planning to include this initially in the RFC but I decided to leave it out eventually, mostly to avoid expanding the scope.

[EliSchleifer]

This baseline approach is also used by rubocop. The problem with these approaches is the baseline files have to be managed by humans and kept up to date.

As we built - our hold-the-line feature built into trunk check. Effectively does this dynamically. So nothing has to be maintained by humans as rules get enabled. Everything get's grandfathered in by any tool that integrates with our runner.

[ondrejmirtes]

• @EliSchleifer It's impressive but I don't like it. Means the user doesn't have insight into pre-existing issues and cannot work to make the issues go away (make the baseline smaller, pay technical debt). Unless I'm missing something you either make the tool to have to look at diffs for the rest of your life, or turn the feature off one day and fix all the pre-existing issues at once (which will never happen).

[nzakas]

I think we've learned from --cache that people will want to have the ability to rename this or place it in a different location. I'd say let's include that capability from the start.

[softius]

@nzakas I will make the necessary changes to the RFC to introduce two new flags --baseline and --baseline-location (instead of one flag called --generate-baseline), so that we maintain consistency with cache flags.

This should allow developers also to skip baseline if needed.

[nzakas]

Thanks for putting this together. It sound good in theory, but you haven't provided much in the way of implementation details. Please dig into the code a bit to see how this might be implemented.

[nzakas]

This looks like a relative path. What is it relative to?

[softius]

Good point. Having a better look at this, it seems that it will better to convert this to a full path.

[nzakas]

I think we've learned from --cache that people will want to have the ability to rename this or place it in a different location. I'd say let's include that capability from the start.

[nzakas]

What caching does this refer to? The --cache-generated file?

[softius]

Yes, it refers to the cache created/used when --cache is specified.

[nzakas]

It seems like you missed the details this comment is asking for. Please look into the code to see how you'd implement such a feature.

[nzakas]

I think the error count is fine for a first pass at this feature.

[nzakas]

To allow for extension in the future, I'd recommend using an object:

Suggested change

	"@typescript-eslint/no-explicit-any": 1
	"@typescript-eslint/no-explicit-any": {
	count: 1
	}

[nzakas]

Just to clarify: we are only tracking problems set to the "error" severity and not anything set to "warn"? If so, please add this into the FAQ section.

[softius]

Correct, we are only counting errors when generating the baseline. Also only errors are considered when checking against the baseline.

[nzakas]

If we use an absolute path, then that means the file can't be checked in because it won't necessarily line up on other systems. Is that okay?

[softius]

No, that would be problematic. Reverting back to the concept of relative paths for portability reasons, where the paths are relative to the CWD. This should cover well cases where multiple paths/globs are provided and seem consistent with other parts.

[bradzacher]

Perhaps relative to the flat config file is better?
The CWD can be somewhat problematic given that you may run eslint from any CWD and it'll figure things out as best as it can.

[nzakas]

I think defaulting to CWD-relative makes sense. People are probably going to run ESLint from the same location each time (the project root).

The flat config file may not always be in the CWD ancestry (if they use -c for instance), so that's unreliable.