refactor: simplify core/lint

[samuelstroschein]

This PR introduces the following changes

All changes intend to reduce complexity, mostly by exposing less features, and wait for user feedback on edge cases before we expose features that might not align with user needs but need to be maintained by us. As a side effect, the DX is expected to increase due to simpler docs.

remove setup ☑️

The createLintRule function is the setup function, see #457 (comment).

remove teardown ☑️

See #457 (comment)

remove the concept of payload ☑️

The concept of payload is redundant due to the createLintRule function.

State exists within the scope of the createLintRule function. There is no need for a dedicated payload concept that needs to be explained and maintained.

const rule = createLintRule("error.rule", "error", () => {

+ const patternIsInvalid = false;

 return {
  visitors: {
   Pattern: ({ target, report }) => {
    report({ node: target, message: "Test" });
+    patternIsInvalid = true;
   },
   Resource: ({ target, report }) => {
+    if (patternIsInvalid) {
     report({ message: "Error in Pattern" });
    }
   },
  },
 };
});

remove the concept of booleans for a rule level ☑️

The concept of passing booleans to a rule level unjustifiably increases comprehension and code-base complexity. There were two reasons for passing booleans to a lint rule:

1. True indicates that the default level of the rule should be used.

This concept has been refactored to use "default" instead.

// uses the standard lint level and passes custom settings to the rule
missingKeyRule(true, { threshold: 4 }),

// uses the standard lint level and passes custom settings to the rule
missingKeyRule("default", { threshold: 4 }),

4. To disable rules.

Disabling rules with a boolean flag is unnecessary. Outcommenting a rule is more intuitive and decreases the complexity of lint.

rules: {
  // disabling a rule
  missingKeyRule(false)
}

rules: {
  // Disabling a rule by commenting it out 😅

  // missingKeyRule()
}

For conditionals, inline conditionals can be used. Given, the syntax is inferior. But, will users use conditionals for rules? If so, they can achieve the desired effect in plain JS. If users strongly demand disabling rules via the lint level, we can re-introduce it.

rules: {
  // disables the rule if it runs on Mondays
  missingKeyRule(!isTodayMonday),
}

rules: {
  // disables the rule if it runs on Mondays
  ...!isTodayMonday ? [missingKeyrule()] : [],
}

replace the concept of context with top-level arguments ☑️

The create lint rule exposes a report function and partial inlang config in the callback with the argument object pattern.

• removes the concept of context which has only been used for report -> less things to explain
• the (partial) config is now exposed as object, allowing us to expose more config options if user request them
• the callback now takes an object of arguments instead of a positional settings argument, allowing us to extend the callback arguments based on user requests.

export const additionalKeyRule: LintRuleInitializer = createLintRule(
  "inlang.additionalKey",
  "warn",
+  ({ report, config }) => {
      visitors: {
        Resource: ({ target }) => {
          if (target && target.languageTag.name === config.referenceLanguage)
            return "skip";
        },
        },
      },
    };
  }
);

Additional information

It seems like complexity originated because the createLintRule was an afterthought [that made original concepts redundant].

[Problem] Not a single end-to-end test existed; the majority of tests had little value

• Over 90% of all tests passed even though the implementation was completely broken at one point.
• No end-to-end test existed.

Many tests used mock rules even though the difference between setting up a mock rule and a real rule is neglectable:

Example 1

used in the tests

const errorRule = {
	id: "error.rule",
	level: "error",
	visitors: {
		Pattern: vi.fn(),
	},
} satisfies LintRule

alternative

const errorRule = createLintRule("error.rule", "error", () => {
 return {
  visitors: {
    Pattern: ({target}) => report({node: target, message: "Error"})
  },
 };
});

Example 2

The following test passed even though the implementation was broken. Visitors have been replaced with an empty object at one point. The test passed regardless due to the artificial testing conditions. A real-world usage would pass object with visitors, subsequently failing this test.

const visitors = {};

const rule1 = createLintRule("my.id", "error", () => {
 return {
  visitors,
 };
});

test("calling the function should return an object with the passed visitors", async () => {
 const configuredRule = rule1();

 expect(configuredRule.visitors).toBe(visitors);
});

Learning

• Prefer end-to-end tests over unit tests.
• Focus on code coverage instead of writing (duplicate) unit tests for every function like "for 'Message' test if 'leave' is not present" and "for 'Pattern' test if 'leave' is not present". The test is identical. Writing and maintaining both tests seems unnecessary. The underlying logic is identical.

[changeset-bot]

🦋 Changeset detected

Latest commit: 2faba51

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@inlang/core	Minor
@inlang/ide-extension	Minor
@inlang/git-sdk	Minor
@inlang/website	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[render]

Your Render PR Server URL is https://inlang-website-pr-459.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cgctj0q84i2237bosppg.

[samuelstroschein]

Open questions

• should lint return errors?

I encountered numerous problems because lint implicitly catches errors. That behavior makes sense. Just because one rule is throwing, the linting process shouldn't abort.

However, debugging became harder and consuming apps like the editor or CLI will never now if a linting process failed.

An easy fix could be to return a tuple:

const [ resources, errors ] = await lint()

But the following pattern wouldn't work anymore (likely doesn't anyways because resources would have type Resource[] not ResourceWithLints[])

let resources = readResources()
resources = await lint(resources)

If we decide for a Result pattern, I propose overhauling the current Result type to be a simple tuple return as well.

• how should the config be passed to a lint rule?

From an API perspective, a second setup function is redundant, but exposing "setup" variables and functions that need to passed outside of the inlang.config.js file is harder.

Option A: A rule from createLintRule returns two functions.

One is to set up in the inlang.config.js file, and the second one is to be called by the linter.

// signature
rule("error", { strict: true })({ config })

// inlang.config.js file
rules: [
  rule("error", { strict: true })
]

// in lint
const processLintRule = async (args: {
	lintRule: LintRuleSetup
	config: Pick<Config, "languages" | "referenceLanguage">
	reference: Resource | undefined
	resources: Resource[]
}) => {
	if (!args.lintRule) return
	const { config, lintRule, reference, resources } = args

        // "second" setup
+++	const rule = lintRule(config)
	
	for (const language of config.languages) {
		await processResource({
			target: getResourceForLanguage(resources, language),
			reference,
			visitors: rule.visitors,
		})
	}
}

Option B: Let the rule handle the config itself

This approach assumes that a rule has access to the config with this. TypeScript lints this even if the config is available. Thus, I don't prefer this approach.

rules: [
  rule("error", { strict: true, languages: this.languages })
]

• out comment lint collections for now?

There are no large collections of rules so far. Explaining and maintaining an extra API because maybe people like it seems unwise. We likely didn't nail the API for collections and should wait for user feedback.

• don't allow rules to define the lint level?

Letting rules define the default lint level in createLintRule leads to complex types, more conditional logic, and the benefit to developers is unclear. I vote to remove the "feature" due to:

• Users face a hidden lint level that might make them wonder why certain things are errors or warnings.
• Rule writers need to think about "should my rule be an error or a warning"?

rules: [
  // implicit behavior. What rules are an error and what are a warning?

  additionalIdRule(),
  brandingRule(),
  grammarRule(),
]

rules: [
  // explicit behavior for close to no additional "cost"

  additionalIdRule("error"),
  brandingRule("warn"),
  grammarRule("error"),
]

[samuelstroschein]

@ivanhofer Props to your type writing skills! Took me hours to understand some but impressive what degree of type safety you get source code. Sometimes going a bit over the edge though ;)

I think the lint design is more future-proof and easier to explain now. Docs are not ready yet. I am awaiting your input on the open questions before I continue.

PS this task shouldn't have taken so long. Was probably not worth it.

[ivanhofer]

I'm sorry that you had to make that much changes. When you told me to create a sophisticated lint process, I included everything that came to my mind to be sure to cover 99% of all needs and hopefully "never" touch it again or introduce breaking changes.

And that's the reason why some things are more complicated then they could be.

In reality you did want a simple linting process.

Some notes on your points:

remove setup

setup is meant for initializing something asynchronously. If you need to make fetch calls to an API, you may need to authorize first. Not every API will have a dedicated npm package. Without the setup function, you would need to write special logic to do the authorization before the first API call. It was also meant to pass options that are meant for the whole linting process.

We could make the setup function optional to still support that use case. Passing those options mentioned above can also happen at another place.

remove teardown

Was meant to free up resources. It is optional an can be used if someone needs it. Nobody is required to use it, if he doesn't need it.

remove the concept of payload

Again, something optional that can be used if someone needs it. It is not required to use it.

Your code will not work, because Pattern is called after Resource. If you did want to support this, we would need the leave functionality again. And for exact such use cases the leave functionality is meant for. Most people won't use it, but if they need it it is there.

Another thing regarding your code example. This will work fine if you just have a single Pattern and Resource, but you will probably have hundreds of Nodes. How do you do that? Write hundreds of variables? Create an array? How do you know what index is the correct one?

With the payload concept you could pass the Resource to the Pattern visitor and directly call the report function there.

Or maybe just throw an error if > 5 Patterns contain bad language. Maybe you are fine with a single one or you are in a migration process and want to be sure that no additional error get's added, but you are temporary fine with the existing ones. But you don't want to fail your CI process.

remove the concept of booleans for a rule level

I would say that most beginner don't know this syntax:

[
	...!isTodayMonday ? [missingKeyrule()] : [],
]

They will probably don't be able to disable rules conditionally. Or at least struggle with it.

I agree true is weird, but we could leave the existing behavior and additionally add the 'default' keyword.

replace the concept of context with top-level arguments

context was meant to be extendable and will have a fix option in the future.

Passing it like you did is a great option. The createLintRule was one of the last additions. Therefore this option was not considered, because everything else was already ready and working.

One "downside" is, that a creator of lint rules is required to use the createLintRule function. But the function is a really good wrapper that handles some stuff for a developer and is is certainly recommended to use it.

No end-to-end test existed

Where would you expect such a test?

linter.ts is the only place where it makes sense and those are e2e tests.

Example 1

createLintRule is (was) not required and calling the function resulted in the same object.

Test was probably written before the createLintRule function was introduced.

Example 2

Why is this test wrong? It just makes sure that the returned object of the callback you add to the createLintRule function get's passed to the resulting lint rule.

Focus on code coverage instead of writing (duplicate) unit tests for every function like "for 'Message' test if 'leave' is not present" and "for 'Pattern' test if 'leave' is not present". The test is identical. Writing and maintaining both tests seems unnecessary. The underlying logic is identical.

If you would refactor the leave function inside Pattern, how would you know that you didn't break the existing behavior? If you don't test it, you won't know.

should lint return errors

It makes sense to return errors instead of just logging them. If it is something exposed externally, a tuple makes sense.

how should the config be passed to a lint rule?

So we now need again a second "setup" function?

Certainly option A.

out comment lint collections for now

It is there and it already works. Sure we can just don't expose it for now and wait until someone demands it.

don't allow rules to define the lint level?

Rules should be configurable! An error for Team A may be just a warning for Team B. Or while migrating something you may wish to just have a warning for a while. And once migration is done, you can change it to an error.

Users face a hidden lint level that might make them wonder why certain things are errors or warnings.

We could say that rules can't specify a default level and everything is a warning, but I would not do that. The additionalKey rule is a good hint that you should take a deeper look, but it should not fail CI, because it does no real harm if it is there.

explicit behavior for close to no additional "cost"

The less an end user needs to do the easier the adoption will be.

Conclusion

I would not remove any functionality. I would just make setup optional, pass those arguments somewhere else and add "default" to the options that a lint rule accepts.

[samuelstroschein]

@ivanhofer

In reality you did want a simple linting process.

I am to blame. As we both figured out, the RFC process did not state goals and non-goals :) Plus, the createLintRule enables most simplifications.

The createLintRule was one of the last additions. Therefore this option was not considered, because everything else was already ready and working.

The createLintRule is awesome! Most simplifications stem from the createLintRule which was added last. I think that is why I used the lints and thought "wait this goes way simpler". The current design reminds me of React class components while the createLintRule resembles functional components.

Regarding this PR, there are some miscommunications

All features that existed before still exist. The mental model for rule writers is easier though by leveraging the createLintRule function.

• Setup is not removed, the createLintRule function is now the setup function. As mentioned in Remove setup from LintRule to ease the writing of rules #457 (comment), an async setup function is not needed. Initializing an API is always sync. Good news: I can turn the createLintRule function async though. As agreed in Remove setup from LintRule to ease the writing of rules #457, I will expose it as sync function for now.

From a DX perspective, the model of writing rules is simpler with just one setup function.

• The concept is payload is replaced but not removed. The enter and leave functions remained identical.

• Rules stay configurable!

My point was that createLintRule shouldn't be able to define a default level because users face implicit lint levels, rule writers need to think about lint level, and the code complexity on our side is growing because LintRule | boolean is required.

---

I finish this version of the PR. Afterward, we can compare the mental model with the current version and decide which one is easier.

[samuelstroschein]

Oh wow, I just stumbled on a great syntax for entering and leaving nodes that is easier to explain and future-proofs the api.

const myRule = createLintRule("foo.x", ({ settings, report }) => {
 return {
  visitors: {
   Resource: ({ onLeave }) => {
+    // the state is local to a visitor without the concept of payloads. just use plain JS.
+    const isError = true;
+    // on leave (and many other features) could be exposed via a callback
+    onLeave(() => {
+     if (isError) report({ node: this, message: "foo" });
    });
   },
  },
 };
});

[ivanhofer]

• Initializing an API is always sync

Not really. Take a look at this: https://auth0.com/docs/secure/tokens/access-tokens/get-management-api-access-tokens-for-production.
You need to retrieve an access token before you can make an authenticated API call.
Making the function (optional) async is not really a big deal, right?

Oh wow, I just stumbled on a great syntax for entering and leaving nodes that is easier to explain and future-proofs the api.

This is a good idea if you want to share state on the same level. I would remove the onLeave parameter and define if a function call get's returned, it is the function that get's called when leaving the node.

Once minor thing that got lost by removing payload is the ability to pass information down to the children. A Pattern does not really know in which Resource it is present.

---

A lot of the things mentioned above were not clear. I'll wait until you are ready with the refactoring.

[samuelstroschein]

Making the function (optional) async is not really a big deal, right?

With the current way I approach this, no. I can make it async. Will do.

I would remove the onLeave parameter and define if a function call get's returned, it is the function that get's called when leaving the node.

This would collide with the concept of returning "skip" and lead to a concept that needs to be explained in general. The callback pattern onLeave is explicit and can be extended with other callbacks over time without introducing new concepts.

A lot of the things mentioned above were not clear. I'll wait until you are ready with the refactoring.

Roger 👍

[samuelstroschein]

NOTE TO MYSELF

I think I found the design improvement that makes lint much simpler. The setup function should return the visitors, not be a separate property. This resembles ESLint and has one major advantage:

The double setup function described in #457 is eliminated without forcing the usage of createLintRule and a double setup function pattern like missingMessageRule("error")(setup). The double setup function pattern would require the config to execute all rules to retrieve static information like the id or level.

// current design
export type LintRule = {
        id: LintRuleId,
        level: LintRuleLevel,
	setup: (args) => MaybePromise<unknown>
	visitors: NodeVisitors
}

// this pull request
export type LintRule = {
        id: LintRuleId,
        level: LintRuleLevel,
	setup: (args) => Promise<Visitors>
}

[samuelstroschein]

@ivanhofer Done ✅

• expose fewer features to not run into the risk of needing to maintain low usage features for years. let's wait for user feedback before we expose more
• heavily lean into object argument pattern to ease future API changes
• return errors during a lint process for apps to display the errors in the UI
• no more 2 setup functions. changing the lint rule as described in refactor: simplify core/lint #459 (comment) was the breakthrough
• don't expose lint test utilities. wait for the merger of CLI re-release #436 which introduces @inlang/core/test