Add the ability to specify mypy options in source file comments

[cpennington]

This still needs work on things like docs and tests, but it seems to basically function.

@gnprice, @ambv: I think this more or less covers what we talked about in the open space today.

[ilevkivskyi]

Also it would be good to show an error if flag is not in self.PER_MODULE_OPTIONS

[cpennington]

Yeah, good call.

[cpennington]

I'm working now on fixing the tests, and on making the # mypy-options: header line use the same commandline arguments as mypy itself.

[cpennington]

This seems related to #1235

[cpennington]

I'm a bit mystified about why the two tests that are failing are. There doesn't seem like there should be anything ambiguous about the BuildManager.errors field.

[gvanrossum]

This is not a full review. I expect that the test failure is because you've disturbed the import order (there are some import cycles in mypy).

[gvanrossum]

Did you see the comment on line 25 above? Please use MYPY instead. (It will somehow not fail in the tests but it will fail in real life when mypy is running on 3.5.1, because TYPE_CHECKING is not in that version's typing.py.)

[cpennington]

Oops. Yes, I definitely missed the earlier comment. I'll fix the uses of TYPE_CHECKING that I added.

[gvanrossum]

There seems to be something very wrong with this PR. Why did you do all the refactoring? It would be much easier to review and easier to understand if you made a minimal change rather than moving all that code into options.

[cpennington]

@gvanrossum: I can certainly roll back the options parsing change, if that would make review easier. The reason that I did it was to make it so that the per-file options could actually use the same format as the CLI. Sadly, parsing those options happened in main.py, so I was attempting to avoid a run-time circular import (main -> options -> main) by moving process_options into options.py.

EDIT: I also tried moving the option parsing into a separate package, but ended up with the same set of circular imports, with just an additional step in the chain. Making the imports of Options be forward-refs seemed like the best solution.

[gvanrossum]

Maybe you​ can first submit a PR that doesn't change any semantics but paves the way for the changes/sharing you have in mind? Then that would hopefully be where we debug why "mypy mypy" fails (which is why the tests are failing).

[cpennington]

Sorry, I'm not sure I understand. You'd like me to separate the re-jiggering of process_options and the actual per-file options overrides, right?

[ddfisher]

Yeah, I think that's what Guido is asking for. (And I agree -- it'd be much easier to review if those were separate PRs.)

I'm a concerned about the correctness of implementing this functionality with a regex -- it would pick up these pragmas from a docstring, for example. It's more work, but I think the correct way forward here is to extend typed_ast to be able to parse arbitrary comment prefixes.

Also, I actually think it would be better here to use the config file format rather than that of command line options, because this directly corresponds to setting a per-file option in the config file. Config file and command line options are not exactly the same -- for example, there's no way to specify ignore_errors = true on the command line. I think having this be a command line flag will also make people more likely to be confused about global flags (like thinking they can use this to turn on incremental on a per-file basis). Finally, writing the pragmas in the config file format greatly simplifies the PR.

[cpennington]

@ddfisher: Ok, I'll make a separate pr for moving process_options, which we can consider separately (and discard if, in fact, we go w/ the config format and don't need it).

[cpennington]

@ddfisher: To be honest, part of the reason for using regexes to pull the initial lines out was because the per-file options updates happen well before the files are actually parsed. Using typed_ast would likely mean either updating the options again after the parsing happens (wherever it does), or parsing the file twice. Fortunately, the particular rules that I was implementing at the time was simple to capture w/ a regex (basically, that the # mypy-options: line had to be part of a comment block at the start of the file, with nothing preceding it except whitespace-only lines). So, there's no worry w/ the current implementation about getting confused by docstrings.

Following the same rules with the config file format, I think I'd instead look for something like:

#  [mypy]
#      disallow_untyped_defs = True

My proposal would be that the [mypy] block must be in the first comment, and that it's terminated by the first line that isn't a comment at the top of the file.

If we do go with this, we'd need to at least move parse_section into mypy/options.py (for the same reason that I moved process_options originally, so I think it would be worth considering doing all of #3391 before this PR (though, right now, something seems horribly wrong over there).

[gvanrossum]

Maybe move it to a new file to avoid circular imports?

[cpennington]

@gvanrossum: Yeah, I tried that when I moved process_options, and the problem was that process_options uses mypy.reports, so there was still circularity. It might work better if I'm only pulling out parse_section, so I'll give that a try.

In aid of that, I'm going to rebase-out the last few commits of this branch, since they are no longer relevant to this particular attempt (and are instead covered by #3391, if we go that direction).

Edit: Sadly, parse_section also uses reporter_classes, which comes from mypy.reports, which uses mypy.options, so the circular import strikes again. I'll see if there's a middleground where I don't have to update all of the users of Options.

[cpennington]

@ddfisher, @gvanrossum: Ok, I've completed a much more minimal extraction of parse_section into mypy/options.py, which allows us to use the config-file format at the start of the file (and it even passes all the tests this time 😃)

Edit: apparently, I was a bit early in declaring victory on the tests.

[ddfisher]

With regards to using typed_ast: there aren't currently any per-file options that affect parsing of a file, so that won't be an issue.

With regards to the format: could you create an issue with a proposal for the format? I think there may be a decent amount of discussion that will need to happen before we decide on something, and I don't want you to have to make big changes to this PR every time the prevailing wind shifts.

[gvanrossum]

@cpennington Should we close this? There has been no activity for 5 months, and the specific design implemented here seems to be superseded by the outcome of the discussion in #3432 (the conclusion there was to use comments of the form

# mypy: some_flag = True

We should probably also close #3391.

[cpennington]

Yeah, let's close this. I can pick it up again and rebase/fix it whenever I get time again.