experimental-inspect: dedup union members - #6273
Conversation
jonasdedden
changed the title
experimental-inspect dedup union membersexperimental-inspect: dedup union members
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks for the PR. Overall seems like the right implementation but I'd prefer the AI clutter thinned down please.
| // A union is built by `reduce`-ing over the members of an enum or the arms of a | ||
| // conversion, each contributing whatever its own `INPUT_TYPE` says. Two of them | ||
| // saying the same thing is normal — `PathBuf` contributes `str | os.PathLike[str]` | ||
| // and `String` contributes `str` — and repeating a member in a written-out union is | ||
| // just noise. The macro cannot collapse it: at that point the members are still | ||
| // unresolved associated constants. |
There was a problem hiding this comment.
this is clearly AI comment which is is overkill here
| // A union is built by `reduce`-ing over the members of an enum or the arms of a | |
| // conversion, each contributing whatever its own `INPUT_TYPE` says. Two of them | |
| // saying the same thing is normal — `PathBuf` contributes `str | os.PathLike[str]` | |
| // and `String` contributes `str` — and repeating a member in a written-out union is | |
| // just noise. The macro cannot collapse it: at that point the members are still | |
| // unresolved associated constants. | |
| // Union deduplication needs to happen here because the macro | |
| // generation only sees unresolved associated constants. |
| /// `A | (B | C)` and `(A | B) | C` both yield `[A, B, C]`. Every operand of a type union is an | ||
| /// atom, a subscript or an attribute, all of which bind tighter than `|`, so re-emitting the | ||
| /// flattened list separated by `|` is the same expression. | ||
| fn flatten_union<'a>(expr: &'a Expr, operands: &mut Vec<&'a Expr>) { |
There was a problem hiding this comment.
Why not make flatten_union also take the "seen" set and do deduplication at the same time?
There was a problem hiding this comment.
Yeah, thanks, now it's better
| /// A union built by `reduce`-ing over enum variants repeats whatever two variants have in | ||
| /// common — `PathBuf` contributes `str | os.PathLike[str]` and `String` contributes `str`. |
There was a problem hiding this comment.
| /// A union built by `reduce`-ing over enum variants repeats whatever two variants have in | |
| /// common — `PathBuf` contributes `str | os.PathLike[str]` and `String` contributes `str`. |
| functions: Vec::new(), | ||
| attributes: vec![Attribute { | ||
| name: "VALUE".into(), | ||
| // `str | os.PathLike[str] | str`, nested to the right the way `reduce` builds it. |
There was a problem hiding this comment.
| // `str | os.PathLike[str] | str`, nested to the right the way `reduce` builds it. | |
| // `str | os.PathLike[str] | str`, nested to the right |
| right: Box::new(right), | ||
| }; | ||
|
|
||
| let module = Module { |
There was a problem hiding this comment.
I think it'd be good enough to just test serialize_expr here and skip the module boilerplate?
f24b2b5 to
d71ee93
Compare
|
Addressed your comments (thanks @davidhewitt and sorry for the verbosity again; was pushing a lot of PRs at one time..). Now the PR hopefully should be a lot cleaner. |
|
Regarding the
+PyAPI_FUNC(PyObject *) Py_CompileStringFlags(
+ const char *str, const char *filename, int start, PyCompilerFlags *flags);It was backported to both 3.14 and 3.13, and shipped in CPython 3.14.7. |
c0e79ca to
788378e
Compare
|
Rebased the branch ontop of your fix, it should hopefully work now |
2 participants
#[derive(FromPyObject)]on an enum builds itsINPUT_TYPEbyreduce-ing the variants' own input types with|. Two ariants overlapping is normal, and the result repeats the overlap:The macro cannot collapse that - at expansion time the members are unresolved associated constants, and it has no idea
PathBufcontributes astrarm. By the timepyo3-introspectionrenders the union it is a plain expression tree, so this flattens the|chain and drops structurally equal repeats, keeping first-occurrence order.Additional formatting improvement: The same match arm also wrote
buffer.push(' ')before the|and nothing after it, so every union came out asstr |None. That is invisible in this repository becausenox -s test-introspectionpipes the stubs throughruff formatbefore comparing them to the checked-in fixtures, but it is what the raw output looks like to anyone who does not format it.