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

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andreas Tolfsen ❲:ato❳]

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)

Comment 1

[Andrew Halberstadt [:ahal]]

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)".

Comment 2

[Andreas Tolfsen ❲:ato❳]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1503599 - [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.

This also adds better error checking for non-int values in mozlint along with a
test.

Comment 4

[Andrew Halberstadt [:ahal]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d9cc09cded0
[mozlint] Default lineno to 0 in result.Issue if unset, r=jgraham

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1503599 - [mozlint] Default lineno to 0 in result.Issue if unset

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.

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/9d9cc09cded0