Infer target version based on project metadata

[stinodego]

Description

Fixes #3124

The logic implemented is as follows:

• If Black's target_version is specified in the pyproject.toml, we ignore the project metadata.
• If it is not specified, we try to read and parse the project metadata to determine a valid list of TargetVersion. If we fail for whatever reason, we continue without a target_version.
• The list of target versions should include all major Python versions allowed by the requires-python specifier.
  • 3.8.5 becomes ["py38"]
  • >3.6,<3.11 becomes ["py37", "py38", "py39", "py310"]
  • <3.7 becomes ["py33", "py34", "py35", "py36"] (Python 3.3 is the minimal supported version for Black)

(for more examples, check the test cases)

Changes:

• Updated parse_pyproject_toml
  • When reading the TOML file, we check if target_version is specified in Black's config. If not, we try to infer it from the project metadata.
  • This seemed like the logical place - since we are reading the file anyway, we might aswell extract the project metadata if we need it.
• Added basic functionality for reading project metadata.
  • Only the PyPA standard is now supported: project -> requires-python.
  • We could add support for Poetry or other tools later.
• Added parsing of the version / specifier based on packaging's Version and SpecifierSet
  • There is some black magic (pun intended) going on in the strip_specifier_set function that I wrote. Unfortunately, working with these kinds of specifiers is nasty when it comes to edge cases. My implementation satisfies all specifier sets I could come up with, but there are probably some corner cases that I did not cover...
• Added packaging>=20.0 as a dependency. This is the minimum version that includes the functionality needed for this implementation.
• Updated the help text for the CLI command.

Checklist - did you ...

• Add a CHANGELOG entry if necessary?
• Add / update tests if necessary?
• Add new / update outdated documentation? --> Updated the CLI argument help text

[ichard26]

Add new / update outdated documentation? --> There isn't good documentation on --target-version, in my opinion. I could add this, but that could be considered out of scope for this PR. Let me know what you prefer!

Drive-by comment, for the time being you could document this in the help for --target-version in the Click CLI setup decorators applied to black.main, but until I get #2839 landed, there are not many places you can document it. Getting that PR landed is on my priority to-do list, but I don't know whether I'll get it in before this PR.

[felix-hilden]

Thanks for the PR! Some small comments below.

[stinodego]

Thanks for the comments so far! I came up with the missing logic for handling some of the special cases. The result isn't super pretty, but I believe it works. It covers all the test cases I could think of.

I set the PR to 'ready to review'. Happy to address any concerns you may have!

[stinodego]

@ichard26 Would you be willing to take another look at this?

We can also conclude that this kind of complexity is not what we want right now, for the small benefit it provides. Then we can close the PR. But would be nice to have some kind of decision on this!

[ichard26]

Hi sorry, I know it's been a long while since I've last said anything. I promise I haven't forgotten this PR, I'll try my best to review this PR in the coming days! On initial thought, I think this will be an useful QOL feature that the extra complexity is worth it.

[stinodego]

I saw I missed a mypy linting error. I rebased the branch and added a commit to fix the issue.

The diff-shades checks are still failing, but I'm not sure how to fix those.

[github-actions]

diff-shades results comparing this PR (1b5b6f4) to main (226cbf0). The full diff is available in the logs under the "Generate HTML diff report" step.

╭─────────────────────── Summary ────────────────────────╮
│ 1 projects & 1 files changed / 2 changes [+1/-1]       │
│                                                        │
│ ... out of 2 395 309 lines, 11 482 files & 23 projects │
╰────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

[stinodego]

Had to rebase due to #3233 . Should be good to go!

[ichard26]

I've been a really bad maintainer, but I promise I've started reviewing this. It overall looks great, I just have a few code style remarks and I want to play around with it locally before approving it. Thank you so much for being patient (way beyond what's reasonable!).

[stinodego]

I've been a really bad maintainer, but I promise I've started reviewing this. It overall looks great, I just have a few code style remarks and I want to play around with it locally before approving it. Thank you so much for being patient (way beyond what's reasonable!).

Don't worry about it! Life gets in the way. We'll just pick it back up whenever you're ready 👍

[ichard26]

Fantastic work overall! Your tests are great and your code was easy to follow. As mentioned earlier, I do have a few stylistic critiques and suggestions to make the logic more robust.

Also two general comments:

• Should we emit a message telling the user what we inferred the target version to be as part of --verbose?

• By inferring a default for --target-version, Black will never do per-file auto-detection if project.requires-python exists and is valid. This is certainly better than the status quo for the lower bound, but it can cause some parsing trouble if the file in question is actually using more modern syntax than specified.¹

  Say for example we have requires-python = ">=3.7". If I format a file that's using 3.10's new match statement, Black will fail with a Cannot parse: ... error because Black decides what grammars it'll try from the target version. While this keeps things consistent, I don't think it makes for a good user experience. The main two ways to fix this are either to infer a list of all supported target versions or actually implement this suggestion to make --target-version a lower-bound by default. The latter is the long-term fix, but I'm not sure when that will be implemented so the former might be necessary as a temporary fix.²

Thanks again for waiting so patiently <3

Footnotes

1. Also Black would never introduce modern syntax that's not permissible under the project's inferred target version even if the file in question is using more modern syntax -- which is probably fine given explicitly providing --target-version works the same way. ↩

2. Unless we decide this is an acceptable failure case and the workaround of explicitly setting --target-version is okay -- which is fine by me ↩

[stinodego]

Thanks for your comments! I just got back from vacation and will take a look at this tomorrow.

[stinodego]

Should we emit a message telling the user what we inferred the target version to be as part of --verbose?

Great suggestion, I will figure out how to do this and add a commit for it.

• By inferring a default for --target-version, Black will never do per-file auto-detection if project.requires-python exists and is valid. This is certainly better than the status quo for the lower bound, but it can cause some parsing trouble if the file in question is actually using more modern syntax than specified. [...] The main two ways to fix this are either to infer a list of all supported target versions or actually implement this suggestion to make --target-version a lower-bound by default. The latter is the long-term fix, but I'm not sure when that will be implemented so the former might be necessary as a temporary fix.

I hadn't considered this; thanks for explaining this. I think it shouldn't be hard to rewrite my logic to generate the full list of target versions. I'll figure this out and add a commit for this.

[ichard26]

Almost there! The problem where Black blows up with invalid python-requires is still there.

--verbose still doesn't say what target version were inferred, but I'm happy to defer this as a future improvement since the per-file autodetection isn't logged anywhere either. A quick note about the changelog otherwise.

Thank you so much for your patience again!

[stinodego]

Hey @ichard26 , sorry I have been sidetracked with some other things - I will complete this sometime next week.

Indeed my two open points are still:

• verbose flag output -> I added this, but I couldn't work out how to nicely specify whether the versions were inferred from project metadata or not. It just prints the versions that are active.
• weird invalid specifiers -> Done; will now default to per-file-autodetection when the metadata field contains invalid specifiers.

I've looked at both but there are some tricky things to consider. I just have to sit down and finish it 😸

[neutrinoceros]

@ichard26 any chance this could make it into the upcoming release ? :-)

[ichard26]

Let's ship this! 🚢 There some tweaks that will probably have to be made, but I'm sick of letting this rot anymore. I'll deal with them in follow up PRs.

It'll be really nice to have this in 23.1.0. Thank you so, so, so much @stinodego for baring with me.

@ ichard26 I addressed your comments, I think this handles all cases properly now. Happy to address any other feedback.

I would also be interested in doing some work as a follow-up to make Black use a single target version, as discussed previously.

That would be wonderful, although as you have noticed, it can get complicated quickly. You've been warned :)

[ichard26]

Wait, it seems like the attrs failure on diff-shades is genuine. I thought it was just a fluke, but it turns out attrs doesn't specify Black's target-version in pyproject.toml so evidently this PR is changing behaviour (in a bad way). I've marked this as draft as I investigate.

[ichard26]

Alright, so the reason why this is breaking attrs is that a non-empty mode.target_versions causes black.parsing.get_grammars() to be stricter with what grammars it returns. I haven't checked, but it's returning only this one probably:

black/src/black/parsing.py

Lines 66 to 69 in f16333e

	# Python 3.7-3.9
	grammars.append(
	pygram.python_grammar_no_print_statement_no_exec_statement_async_keywords
	)

I miscontextualized how get_grammars() works. I thought as long as the 3.10 grammar might be supported, it'll be added to the list to try... that's not the case! Instead it checks for strict compatibility (using black.mode.supports_feature()) so if there's a target-version that doesn't support the 3.10 grammar, it won't be added.

Attrs is formatting fine right now because if mode.target_versions is empty, all grammars get picked:

black/src/black/parsing.py

Lines 50 to 59 in f16333e

	if not target_versions:
	# No target_version specified, so try all grammars.
	return [
	# Python 3.7+
	pygram.python_grammar_no_print_statement_no_exec_statement_async_keywords,
	# Python 3.0-3.6
	pygram.python_grammar_no_print_statement_no_exec_statement,
	# Python 3.10+
	pygram.python_grammar_soft_keywords,
	]

So technically this PR doesn't introduce any more regressions, but it just exposes this pitfall... I'm not entirely sure what the best way of fixing this is. My suggestion would be to add the 3.10 grammar if any of the target versions support Feature.PATTERN_MATCHING, not all.

[ichard26]

[attrs - https://github.com/python-attrs/attrs.git]
╰─> [revision 9d23ae9536beb3cd30241f05123190ec1ecc20c1](https://github.com/python-attrs/attrs/tree/9d23ae9536beb3cd30241f05123190ec1ecc20c1)
--- a/attrs:src/attr/validators.pyi
+++ b/attrs:src/attr/validators.pyi
@@ -80,7 +80,7 @@
 def min_len(length: int) -> _ValidatorType[_T]: ...
 def not_(
     validator: _ValidatorType[_T],
     *,
     msg: Optional[str] = None,
-    exc_types: Union[Type[Exception], Iterable[Type[Exception]]] = ...
+    exc_types: Union[Type[Exception], Iterable[Type[Exception]]] = ...,
 ) -> _ValidatorType[_T]: ...

I don't have time to investigate, but presumably this is due to better inference of the target version. Not ideal, but as long as we land this in 23.1.0 where we're changing the stable style anyway, it should be fine.

[ichard26]

Thanks again @stinodego. It's been great working with you 🖤

[stinodego]

Thanks for putting the finishing touches on this, @ichard26 ! Glad to have this part of the latest release.

If any issues pop up, do not hesitate to tag me.