Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change representation of match as / capture as Name(..., ctx=Store()) #88160

Open
asottile mannequin opened this issue Apr 30, 2021 · 10 comments
Open

change representation of match as / capture as Name(..., ctx=Store()) #88160

asottile mannequin opened this issue Apr 30, 2021 · 10 comments
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@asottile
Copy link
Mannequin

asottile mannequin commented Apr 30, 2021

BPO 43994
Nosy @gvanrossum, @ncoghlan, @asottile, @brandtbucher, @freundTech, @cdce8p

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-04-30.22:49:51.192>
labels = ['3.10']
title = 'change representation of match as / capture as `Name(..., ctx=Store())`'
updated_at = <Date 2021-08-08.10:38:09.417>
user = 'https://github.com/asottile'

bugs.python.org fields:

activity = <Date 2021-08-08.10:38:09.417>
actor = 'cdce8p'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2021-04-30.22:49:51.192>
creator = 'Anthony Sottile'
dependencies = []
files = []
hgrepos = []
issue_num = 43994
keywords = []
message_count = 10.0
messages = ['392531', '392547', '392548', '392552', '392562', '392565', '392566', '392568', '392571', '392612']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'ncoghlan', 'Anthony Sottile', 'brandtbucher', 'freundTech', 'cdce8p']
pr_nums = []
priority = 'high'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue43994'
versions = ['Python 3.10']

@asottile
Copy link
Mannequin Author

asottile mannequin commented Apr 30, 2021

I'm looking at adding support to match for pyflakes, and the first impression I have is that MatchAs is unnecessarily different from Name with ctx=Store()

if it were represented as the latter pyflakes would not require special handling of match, it would work the same as the current code

I suspect other static analysis tools would benefit from a change as well

@asottile asottile mannequin added 3.10 only security fixes labels Apr 30, 2021
@brandtbucher
Copy link
Member

Do you mind providing a bit more context? I'm sort of confused what exactly is being proposed here (as far as I can tell, it's not really possible to represent "<pattern> as <name>" with a single Name node).

Also, I'm not sure if you're aware, but we just completed a huge overhaul of the ASTs generated for match statements. This work was specifically done for clients of the AST like mypy. See the discussion at: python/mypy#10191 and in bpo-43892.

The new nodes are documented here: https://docs.python.org/3.10/library/ast.html?highlight=matchas#pattern-matching

@brandtbucher
Copy link
Member

FWIW, I'm never used pyflakes, but I'm not really sure how it would be able to provide useful linting by just treating patterns as expressions (which is what I assume is desired here).

I assume that these are the three lines you're trying to get rid of?

https://github.com/asottile/pyflakes/blob/silly_match_statement/pyflakes/checker.py#L2390-L2392

@asottile
Copy link
Mannequin Author

asottile mannequin commented May 1, 2021

I'm suggesting instead of:

MatchAs(pattern=None, name='foo')

to have

MatchAs(pattern=None, name=Name('foo', ctx=Store()))

@asottile
Copy link
Mannequin Author

asottile mannequin commented May 1, 2021

and actually, now that I look close it would be useful for MatchStar and MatchMapping to also use a Name(..., ctx=Store()) for their respective parameters as well

@brandtbucher
Copy link
Member

+ Nick and Guido

The only benefit I see on our side is that it leaves the door open for complex assignment targets in the future, like (a, b), a[b], etc.

(If I recall correctly, this is also why NamedExpr uses an expr target rather than just an identifier.)

I guess I'm neutral on this. The usability argument seems weak, but I can see the separate case for forward-compatibility with possible syntax extensions in the future. Thoughts?

Marking as high priority since we need to make a decision on this before the beta.

@gvanrossum
Copy link
Member

I'm -0.5 on reusing Name(ctx=Store). The reason we're using that in assignments is that in the past (before the PEG parser) the parser literally used the same grammar for the LHS and RHS of assignments, and a second pass was used to rule out invalid targets (like "1 = e" or "f() = a") and mark the Name nodes representing targets as stores.

We don't reuse Name nodes for other binding targets like parameters or imports.

PS. To reach Mark it's best to email him directly, he's not very responsive to GitHub notifications (there are too many).

PPSS. I'm on vacation until May 8 and am trying not to check my email much. But I already failed on the first day. :-)

@asottile
Copy link
Mannequin Author

asottile mannequin commented May 1, 2021

at least for static analysis of other python constructs it's very convenient to know from a Name node alone whether it's being used in a read or write context -- without this information an ast traversal needs to maintain more information about whether it's a read or write context

for pyflakes this is especially important as it needs to know what names are defined in scope (and referenced in scope) to produce diagnostic messages

for other tools like dead / vulture it's useful to identify introduced and referenced names similarly

the as in with does and the target for assignment expressions so I would expect the similar constructs in match to do so as well

Name nodes are also useful for better diagnostic messages as they contain positioning information, which isn't easily extracted from MatchAs, etc. -- if I recall correctly, the asname for imports was recently extended to add this information for the same purpose

@freundTech
Copy link
Mannequin

freundTech mannequin commented May 1, 2021

I already brought this up on the main pattern matching issue some time ago (https://bugs.python.org/issue42128#msg388554), where the consensus was that not using a Name is consistent with other parts of the ast, such as import ... as identifier, except ... as identifier and others.

For mypy having a Name node there would slightly simplify the code (I'm currently inserting a dummy NameExpr at AST-Conversion.

+0 from me.

@gvanrossum
Copy link
Member

Honestly if someone manages to get a PR in I won’t be a spoilsport. So make
me +0.--
--Guido (mobile)

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Development

No branches or pull requests

3 participants