Closed Bug 1503599 Opened 1 year ago Closed 1 year ago

TypeError: int() argument must be a string or a number, not 'NoneType' on running WPT linter

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: ato, Assigned: ahal)

References

Details

Attachments

(1 file, 1 obsolete file)

When the WPT linter produces a lint error without a line number,
mozlint passes None to the built-in int() coercion function, resulting
in the warning seen below.

This can be reproduced by creating a file with some linting mistakes
in it and running "./mach lint -lwpt":

> % echo 'print "foo"' >testing/web-platform/tests/webdriver/foo.py
> % ./mach lint -lwpt testing/web-platform/tests/webdriver/foo.py
> Traceback (most recent call last):
>   File "/home/ato/src/gecko/python/mozlint/mozlint/roller.py", line 39, in _run_worker
>     res = func(paths, config, **lintargs) or []
>   File "/home/ato/src/gecko/python/mozlint/mozlint/types.py", line 48, in __call__
>     return self._lint(paths, config, **lintargs)
>   File "/home/ato/src/gecko/python/mozlint/mozlint/types.py", line 132, in _lint
>     return func(files, config, **lintargs)
>   File "/home/ato/src/gecko/tools/lint/wpt/wpt.py", line 47, in lint
>     proc.returncode))
>   File "/home/ato/src/gecko/python/mozlint/mozlint/result.py", line 133, in from_config
>     return Issue(**attrs)
>   File "/home/ato/src/gecko/python/mozlint/mozlint/result.py", line 85, in __init__
>     self.lineno = int(lineno)
> TypeError: int() argument must be a string or a number, not 'NoneType'
> A failure occurred in the wpt linter.
> ✖ 1 problem (0 errors, 0 warnings, 1 failure)
AIUI, this is happening because the wpt linter is linting the file name itself and therefore doesn't have a line number to give. Then mozlint is assuming that the line number is present with that call to `int` (which is a recent regression).

This raises a somewhat fundamental question about what mozlint is and isn't. Do we want to encourage and make it easier for linters that don't have a line number? There is a lot of code that kind of assumes that there is (e.g formatters and opening errors in an editor are two examples of places that make this assumption).

There are two options:
1) Make linters that don't use line numbers first class
2) Use a lineno of 0 to denote a file level issue and add better error checking to enforce int in mozlint

I'm kind of tempted to just do 2 for now and investigate doing 1 properly in a follow-up bug. Solution 1 is a bit more work than I want to spend on this right now. Plus solution 2 would still be an improvement on the old (non-busted) behaviour where we were getting formatted output that looked like "None An error occurred (wpt)".
Assignee: nobody → ahal
Status: NEW → ASSIGNED
It seems perfectly reasonable to want to emit linter warnings about
the general state a file is in, without pin-pointing the problem
to a specific line in that file.  For a file-level error in WPT, I
would expect the following:

> % ./mach lint -funix -lwpt testing/web-platform/tests/foo.py
> testing/web-platform/tests/foo.py: this file does not conform to whatever WPT specific reason

If I saw this instead, I would be confused:

> testing/web-platform7tests/foo.py:0: this file does not conform to whatever WPT specific reason

I guess I don’t exactly see why mozlint can’t take a NoneType and
skip the line number coercion with int(), but if the proposal is
to pass in 0 and have the formatter/output skip writing ":0", that
would also satisfy my needs.

I don’t have any particular opinion how this is fixed internally,
for as long as the output that is present to users makes sense.
This assumes that a lineno of 0 denotes a "file-level" issue, e.g an issue
associated with the filename. In the future it might be better to treat these
"file-level" issues as first class citizens. This would involve updating things
like formatters, editor integrations and review bot to not assume a lineno.

This also adds better error checking for non-int values in mozlint along with a
test.
See Also: → 1503606
Yeah, I think my main issue is one of time. It's not just formatters, it might affect the reviewbot or some editor integrations as well. Keeping at as an int just reduces the risk of failure.

I'm definitely on board with the formatters detecting lineno=0 and omitting the ":0". I'm just saying I'm not going to be fixing that in this bug.

Note that the previous behaviour was to print:
testing/web-platform7tests/foo.py:None: this file does not conform to whatever WPT specific reason

So at the very least ":0" is not any worse :).

p.s I filed bug 1503606 as follow-up, though don't expect to have much time to work on it.
Attachment #9021552 - Attachment description: Bug 1503599 - [lint] Default lineno to 0 in wpt linter, r?jgraham → Bug 1503599 - [mozlint] Default lineno to 0 in result.Issue if unset, r?jgraham
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d9cc09cded0
[mozlint] Default lineno to 0 in result.Issue if unset, r=jgraham
This assumes that a lineno of 0 denotes a "file-level" issue, e.g an issue
associated with the filename. In the future it might be better to treat these
"file-level" issues as first class citizens. This would involve updating things
like formatters, editor integrations and review bot to not assume a lineno.
https://hg.mozilla.org/mozilla-central/rev/9d9cc09cded0
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attachment #9023333 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.