Consider support for output of data in rules

[jonathanKingston]

To make rules more portable between projects it would be useful to be able to get an output of data attributes on failures.

Currently https://github.com/mozilla/addons-linter is taking rules from other projects and may have to parse them in some cases to output some human readable form of the issue.

This is an issue because some rules output multiple string messages and the nice output of these strings would be vastly different or not relevant to the addons-linter.

If the linter could look at some form of 'meta data' for each message object returned by the CLI engine the following code wouldn't have to hard code error string replacements which makes it somewhat inflexible: https://github.com/mozilla/addons-linter/blob/master/src/scanners/javascript.js#L37

Take for example my rule:
https://github.com/jonathanKingston/eslint-plugin-no-unescaped/blob/master/lib/rules/enforce.js#L146

I might output the following errors via reports:

context.report({
  node,
  message: `Unexpected use of unescaped assignment to ${propertyKey}`,
  data: {
   errorId: "unescaped-assign",
   propertyKey
  }
})

and

context.report({
  node,
  message: `Unexpected use of unescaped call to ${methodName} at index ${index}`,
  data: {
   errorId: "unescaped-call",
   index,
   methodName
  }
})

and

context.report({
  node,
  message: "Unexpected type passed to call expression",
  data: {
   errorId: "unexpected-expression"
  }
})

That way an addon linter could ignore "unexpected-expression" as they are failings in the rule rather than the others. They could also choose to express the message for "unescaped-call" and "unescaped-assign".

Addon linter is obviously designed for developers to upload code to https://addons.mozilla.org/ which would like to give help on how to fix or advice why something might be banned.

[ilyavolodin]

@jonathanKingston I'm not really sure what you are asking for. Could you clarify what changes you would like to see? Do you mean that you would like to get not only compiled error string, but also components that it was compiled from?

[not-an-aardvark]

Ping; @jonathanKingston, would you mind clarifying your request to answer @ilyavolodin's questions?

[jonathanKingston]

Sorry missed this.

Could you clarify what changes you would like to see?

In the executeOnFiles and executeOnText the result messages would contain granular error properties above what a string would provide.

The documentation shows this example:

{
    results: [
        {
            filePath: "/Users/eslint/project/myfile.js",
            messages: [{
                ruleId: "semi",
                severity: 2,
                message: "Missing semicolon.",
                line: 1,
                column: 13,
                nodeType: "ExpressionStatement",
                source: "\"use strict\"", // Deprecated: see "please note" paragraph below.
                fix: { range: [12, 12], text: ";" }
            }],
            errorCount: 1,
            warningCount: 0,
            source: "\"use strict\"\n"
        }
    ],
    errorCount: 1,
    warningCount: 0
}

This would be changed to perhaps something like:

...
            messages: [{
                ruleId: "semi",
                severity: 2,
                message: "Missing semicolon.",
                line: 1,
                column: 13,
                nodeType: "ExpressionStatement",
                source: "\"use strict\"", // Deprecated: see "please note" paragraph below.
                fix: { range: [12, 12], text: ";" },
                data: {
                  type: "string-expression",
                  eol: true
                }
            }],
...

However the context.report() examples in my initial comment explain the use-case a lot better than this tenuous example.

Do you mean that you would like to get not only compiled error string, but also components that it was compiled from?

Yeah exactly that, currently we have the following plugin: no-unsanitized

We would like to differentiate between:

• an unsupported expression by the rule itself, only thrown if ESTree gets more syntax
• a code error that we would like to present to an addon author

An external repository wants to check for these errors would like to explain the error in much more detail when they are thrown (this repo is for uploading of an addon to addons.mozilla.org and may not be an addon developer themselves) or also ignore certain issues.

So:

this.context.report(node, `Unsafe call to ${this.getCodeName(node.callee)} for argument ${propertyId}`)

May get presented on addons.mozilla.org as:

Hey, it looks like you have a coding error that we don't currently permit in the addon store. This is to protect users of Firefox being vulnerable to security flaws. As part of our automated checks we found something that looks like it would be in this category.

We found the ${propertyId}${ordinal} argument to ${node.callee} in ${file}#${line} to be an issue.

Obviously having the linting rule as verbose as that would be nasty for normal CLI usage, so we would like a way to parse the params of the error itself.

Let me know if this makes sense. Thanks.

[not-an-aardvark]

Thanks for clarifying.

Would #6740 solve this issue? It seems like you need a way to get more info about short "CLI-suitable" messages to display more verbose messages on addons.mozilla.org. If #6740 was implemented, you could look in the executeOnText output for a message ID, and then map different message IDs to different verbose outputs.

[jonathanKingston]

@not-an-aardvark sorry for the delay. I'm not sure this would be good enough to solve the issue cleanly.

Perhaps if the optional data object was permitted only if a message id was provided? This would permit us to use shapes like:

{
  messageId:1,
  message: "Expression not supported of type: {{type}}",
  data: {
    type: expression.type
  }
}

I also don't mind if it is data either, just seemed that was useful as it makes up components of the string itself.

/cc @muffinresearch and @EnTeQuAk as you likely will deal with this more than me do you have other thoughts?

The ruleHelper I worked on has the .report() examples I know of that are likely more complex than most:

Lines not solvable by message id:

this.context.report(node, `Unsafe call to ${this.getCodeName(node.callee)} for argument ${propertyId}`);
this.context.report(node, `Error in no-unsanitized: ${reason}. Please report a minimal code snippet to the developers at ${bugPath}`);
this.context.report(node, `Unsafe assignment to ${node.left.property.name}`);

Solvable by message id:

this.context.report(node, `Method check requires properties array in eslint rule ${methodName}`);

[not-an-aardvark]

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get enough support from the team and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after a long time tend to never do it, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.