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

WIP: Validate rule options #2179

Closed
wants to merge 12 commits into from
Closed

WIP: Validate rule options #2179

wants to merge 12 commits into from

Conversation

btmills
Copy link
Member

@btmills btmills commented Mar 30, 2015

This is a code-included, tests-passing request for bikeshedding that might address the discussions in #967 and (eventually) #1568. This is not ready for a line-by-line code review until higher-level concepts have been addressed and solidified.

With this approach, validation is optional. Rules opt in by exporting a schema object describing a JSON schema for the options the rule accepts. Most rules have very similar option formats, so this wasn't difficult at all.

Example

// test.js
var foo = 4 == (2 * 2);
// config.json
{
    "rules": {
        "eqeqeq": [2, "samrt"]
    }
}
$ eslint --reset --no-eslintrc -c config.json test.js

test.js
  0:0  error  Invalid configuration: ["samrt"] must be an enum value  eqeqeq

✖ 1 problem (1 error, 0 warnings)

Performance

So far, I've created schemas for all rules that are enabled by default in order to get a good picture of the performance impact, which, happily, turned out to be minor.

master:

CPU Speed is 2200 with multiplier 7500000
Performance Run #1:  1994.5480929999999ms
Performance Run #2:  2227.983234ms
Performance Run #3:  2227.137346ms
Performance Run #4:  2177.492682ms
Performance Run #5:  2025.890174ms
Performance budget ok:  2177.492682ms (limit: 3409.090909090909ms)

This PR:

CPU Speed is 2200 with multiplier 7500000
Performance Run #1:  2228.800724ms
Performance Run #2:  2247.415942ms
Performance Run #3:  2255.482898ms
Performance Run #4:  2244.724581ms
Performance Run #5:  2235.863643ms
Performance budget ok:  2244.724581ms (limit: 3409.090909090909ms)

Note that these performance numbers are for linting a single file. When linting a whole directory, part of the validation cost is amortized over all files because is-my-json-valid uses code generation to create the validators, which are generated once, cached, and reused.

Configuration guidelines

These are some principles that we seem to prefer, but I'd like to consider formalizing them. These are just ones I've noticed or think make sense; feedback is requested.

  1. All rules should have sensible defaults, such that they can be enabled and will do something without having to specify any options.

    Example: "quotes": 2 can assume double quotes since neither mode was specified.

  2. A rule whose primary option is a mode may specify that mode using a descriptive string as the first option.

    Example: "quotes": [2, "single"] intuitively should enforce single quotes.

  3. All other (non-mode) options should be named properties of a single options object. This helps avoid confusion as to the order of parameters, and it allows some to be left as default while only specifying deviations from the defaults.

    Example: "semi-spacing": [2, { "before": false, "after": true }]

  4. If we have validation, out of range values should fail validation. Previously, some rules, comma-dangle being one, considered unknown values to mean the default.

    Example: "quotes": [2, "snigle"] should fail, since clearly "single" was intended, yet assuming the default of "double" would lead to confusing behavior until the error was noticed. (This is a trivial example. Other, deeper options fail silently and are much harder to debug.)

Some rules still support for deprecated options. For example, no-mixed-spaces-and-tabs also accepts a boolean value, though it's not documented, and a source code comment suggests adding a deprecation warning. Since this would be a breaking change, it could also be a good time to remove support for deprecated options concurrently, though I've not done so here.

Thoughts?

Feedback requested

  • General bikeshedding: is this the right approach?
  • What should error messages look like? I'm not attached to what exists currently; I think they could be more helpful.
  • Simplifying schemas: since there's so much in common, it might be feasible to offload some of the schema-creation work to the validator.
  • Are there guiding principles anywhere for how rules should be configured? I've sketched out some ideas above; are they reasonable?

@ilyavolodin
Copy link
Member

In general approach looks pretty good to me. I would make a few changes, like, adding possible error message into schema as well as exposing a function on cli to dump all schemas for all rules, but all of that can be added later as an independent commit (this would allow us to automatically improve rules documentation as well as allow editors to get all possible configurations for rules).
My only question is, should we validate configuration as part of a normal run, or just have a command line option that will validate configuration, without actually validating code?

@ilyavolodin
Copy link
Member

Also, one more thing, I'm not sure that would be possible, but it would be nice if the error message would point to the configuration file with line number (don't think it is, since it's inside the loop that processes files, so it will always point to the current file that is being validated).

@nzakas
Copy link
Member

nzakas commented Mar 30, 2015

I think the idea is correct, just not sure about the implementation. The schema looks a bit complicated, which means we would need a schema validator, and that starts to get hairy. I'm wondering if we should use something like joi instead.

@ilyavolodin I definitely don't want a separate validation step, it should be fine as part of the normal run, otherwise people will rarely be aware of errors.

I'm not sure about the error representation either. The error could be in an eslintrc file or a JS file due to inline comment. We either need to be better about reporting where the error occurred or just throw the error and let ESLint crash.

@btmills
Copy link
Member Author

btmills commented Apr 7, 2015

it would be nice if the error message would point to the configuration file with line number

// test.js
/* eslint quotes: [3, "double"] */
var answer = 6 * 7;
// config.json
{
    "rules": {
        "semi": [2, "laways"]
    }
}
$ eslint test.js
... error thrown

Error: test.js line 2:
    Configuration for rule "quotes" is invalid:
    Error level must be between 0 (off) and 2 (warning).

... stack trace
$ eslint -c config.json test.js
... error thrown

Error: /Users/brandon/code/eslint/eslint/config.json:
    Configuration for rule "semi" is invalid:
    Value "laways" must be an enum value.

... stack trace
$ eslint --rule 'eqeqeq: [2, "smart", "asdf"]' test.js
... error thrown

Error: CLI:
    Configuration for rule "eqeqeq" is invalid:
    Value "2,smart,asdf" has more items than allowed.

... stack trace

The schema looks a bit complicated... I'm wondering if we should use something like joi instead.

I looked into joi, but I found no way to programmatically inspect schemas. Right now we only need validation, but getting into configuration setup and fuzz testing, that becomes more important. I did, however, offload some of the schema creation onto the validator. Half of the schemas now become just an enum, semi being one example:

module.exports.schema = [
    {
        "enum": ["always", "never"]
    }
];

As for Travis, it's failing because of code coverage; I didn't break any logic.

@nzakas
Copy link
Member

nzakas commented Apr 8, 2015

Here's my concern: representing data as JSON is a solved problem. If we're creating a new schema type, then we're also on the hook for creating something to validate that schema. I'd much prefer using an existing solution vs. creating something completely new.

@ilyavolodin
Copy link
Member

Just in case you change the format of schema to something else, please keep in mind I would like to add the following information to the schema later at some point. Here's the information that I would like to add:

{
  "description": "very short description of the rule",
  "errors": ["list of all of the errors that can be reported by this rule"],
  "options": [ { "always": "description", "never": "description"} ]
}

Or something like that. This would allow us to enhance our documentation automatically, as well as provide a way to extract all of the options for all rules for integrations and things like web .eslintrc builder.

@nzakas
Copy link
Member

nzakas commented Apr 8, 2015

@ilyavolodin we can always add extra properties to the rules themselves, this doesn't have to be part of the schema

@doberkofler
Copy link
Contributor

@btmills I love the idea to have the rules validated and it's great what you have prepared.
I might be far off but why could not just each rule (maybe also optionally) implement a callback that is responsible to validate it's own configuration and to report any errors.
I see the advantages in a standardized schema but it also seems a little complicated and it still requires the schema's themselves to be validated as @nzakas points out.

@btmills
Copy link
Member Author

btmills commented Apr 15, 2015

I'd much prefer using an existing solution vs. creating something completely new.

@nzakas This is still using unmodified JSON schema. However, since much of each rule's schema was redundant, I had the rules just return the items in the options array, and makeSchema wraps that, inserting the reporting level as the first item and specifying type, minItems, maxItems. A rule's schema property is an array of individual JSON schemas for each option, and the result of makeSchema is a full JSON schema for the rule's options.

For example, quotes exports the following array, each element of which is a valid JSON schema by itself:

[
    {
        "enum": ["single", "double"]
    },
    {
        "enum": ["avoid-escape"]
    }
]

Wrapping it with makeSchema produces the full JSON schema for all of the rule's options (ignore the title and description attributes for now):

{
    "title": "quotes",
    "description": "Specify whether double or single quotes should be used.",
    "type": "array",
    "items": [
        {
            "enum": [0, 1, 2],
            "description": "Reporting level - 0 for off, 1 for warning, or 2 for error."
        },
        {
            "enum": ["single", "double"],
            "description": "Enforce single or double quotes."
        },
        {
            "enum": ["avoid-escape"],
            "description": "Avoid escaped quotes inside strings by allowing both quote types when necessary."
        }
    ],
    "minItems": 1,
    "maxItems": 3
}

Because there are valid schemas in both phases, we could validate them against JSON schema's schema in either place.

I would like to add the following information to the schema later at some point.

@ilyavolodin I annotated the above example with title and description attributes, both of which are purely informative and part of the JSON schema spec. Would those be sufficient for what you're imagining? (The overall description I pulled straight from the docs/rules index. I could see it being exported by the rule as a description property to be used in a configuration helper akin to #2302. It would also enable us to generate docs/rules/README.md programmatically. The list of possible error messages might be exported similarly.)

why could not just each rule (maybe also optionally) implement a callback

@doberkofler Callbacks were actually the first thing I considered way back when I started working on fuzz testing, see #1568 (comment). However, as pointed out by @nzakas, fuzz testing and config validation would require two separate callbacks. We'd need another for some sort of autoconfiguration utility. That's why we need some standard data format that facilitates programmatic inspection and generation. From what I've seen, JSON schema seems to be that.

@ilyavolodin
Copy link
Member

@btmills Yes, that would work perfectly for auto configuration and auto documentation. One last thing that would be nice to add is some sort of way to mark default options (if there are any).

@nzakas
Copy link
Member

nzakas commented Apr 16, 2015

Ah gotcha! Will have to take a closer look.

@ilyavolodin
Copy link
Member

@nzakas Any updates. Would love to get this into 1.0.0

@btmills
Copy link
Member Author

btmills commented Apr 24, 2015

One last thing that would be nice to add is some sort of way to mark default options (if there are any).

module.exports.defaults = ["double"]; perhaps? A side benefit of that would be trivial validation against the schema as part of npm test.

@ilyavolodin
Copy link
Member

@btmills That would work.
Sorry forgot about one more thing (not really important for validation of the configuration, but would be very helpful for auto documentation). List of possible error messages reported by the rule. Just a top level property errorMessages of type array would work just fine for that.

@nzakas
Copy link
Member

nzakas commented Apr 25, 2015

Can we just use what's in eslint.json as the defaults?

@ilyavolodin
Copy link
Member

You are correct. I totally forgot about eslint.json. I can just use that for defaults. Are you fine with the rest of it? Can it be merged (after build breakage fixes)?

source, ":\n",
"\tConfiguration for rule \"", id, "\" is invalid:\n",
"\tError level must be between 0 (off) and 2 (warning).\n"
].join(""));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 is error, not warning

@nzakas
Copy link
Member

nzakas commented Apr 25, 2015

Yeah, I think in general the approach is sound. I'd be interested in seeing the perf difference of running on this repo both with and without validation.

@IanVS
Copy link
Member

IanVS commented Apr 29, 2015

I'm excited to see this happen. Is there any way I can help?

@btmills btmills force-pushed the schema branch 2 times, most recently from 29966e8 to 82e3194 Compare May 4, 2015 04:01
btmills added a commit to eslint/eslint-tester that referenced this pull request May 5, 2015
@btmills
Copy link
Member Author

btmills commented May 6, 2015

I'd be interested in seeing the perf difference of running on this repo both with and without validation.

npm run perf reports 2112ms on master and 2145ms on this PR.

I think this is almost done. Just a couple outstanding concerns:

  • Currently there's no way to test plugin schemas because plugins in a config file aren't loaded until configs are merged, which would mean losing the location of any errors. I can do this, but it would require reworking some of the config merging and plugins system.
  • There needs to be a way to validate options passed through eslint-tester. I have a commit linked above that hacked it together long enough for me to catch bad test options, but there has to be a better solution. At the very least, there needs to be a way to get a rule's schema.

Thoughts?

@nzakas
Copy link
Member

nzakas commented May 6, 2015

Hmm, not too bad perf-wise.

Can you explain more about the plugin problem? I think I get what you're saying, but I'm not entirely sure.

@ilyavolodin
Copy link
Member

@btmills There's a ticket opened for adding addPlugin to CLI here: #1971 we need that to enable testing of processors, that should also cover testing of plugin schemas, right?

@btmills
Copy link
Member Author

btmills commented May 7, 2015

@nzakas Since we want to report the location of invalid options, they have to be checked against the schema in loadPlugin before configs are merged and the location is lost. When a config file specifies a plugin and one of its rules, as in the example below, the validator will attempt to load the schema for the rule react/display-name, but that rule will be unavailable until the react plugin is loaded later.

{
    "plugins": [
        "react"
    ],
    "rules": {
        "react/display-name": 1
    }
}

@ilyavolodin Hmm from what I read I think #1971 is separate from the particular issue discussed above, though it would help plugin authors test schemas from eslint-tester.

btmills added a commit that referenced this pull request May 13, 2015
btmills added a commit that referenced this pull request May 13, 2015
@btmills
Copy link
Member Author

btmills commented May 13, 2015

The first part is at #2523.

btmills added a commit that referenced this pull request May 13, 2015
btmills added a commit that referenced this pull request May 13, 2015
btmills added a commit that referenced this pull request May 13, 2015
btmills added a commit that referenced this pull request May 18, 2015
btmills added a commit that referenced this pull request May 18, 2015
btmills added a commit that referenced this pull request May 18, 2015
btmills added a commit that referenced this pull request May 18, 2015
ilyavolodin added a commit that referenced this pull request May 20, 2015
New: Add config validator (refs #2179)
btmills added a commit that referenced this pull request May 20, 2015
btmills added a commit that referenced this pull request May 20, 2015
btmills added a commit that referenced this pull request May 20, 2015
ilyavolodin added a commit that referenced this pull request May 21, 2015
@btmills
Copy link
Member Author

btmills commented May 21, 2015

Now that we have the validator from #2523 and schemas from #2580, we can enable automatic validation. How should eslint-tester verify the tests?

@nzakas
Copy link
Member

nzakas commented May 22, 2015

Can you open an issue for that discussion? I'd like to get out conversation back on issues and close this.

@btmills
Copy link
Member Author

btmills commented May 22, 2015

Closing in favor of #2595.

@btmills btmills closed this May 22, 2015
btmills added a commit to eslint/eslint-tester that referenced this pull request Jun 4, 2015
btmills added a commit to eslint/eslint-tester that referenced this pull request Jun 4, 2015
btmills added a commit to eslint/eslint-tester that referenced this pull request Jun 4, 2015
ilyavolodin added a commit to eslint/eslint-tester that referenced this pull request Jun 15, 2015
Breaking: Validate test options (refs eslint/eslint#2179)
@btmills btmills deleted the schema branch June 30, 2015 17:47
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants