Issue 1458 #3879
Issue 1458 #3879
Conversation
|
Actually, now I'm wondering if it would be simpler to add a set of node-IDs-for-exprs-that-should-be-parenthesized to the type context. That would avoid the awkwardness over "if e has some property, does (e) also have that property?" |
|
The adjustments table is created by the type checker whenever an auto-deref or auto-ref occurs (field access, method access, indexing). It indicates the location of these automatic adjustments. The interaction between an adjustment and parentheses will depend on how you modified the type checker. It seems that you are "deparenthesizing" at various places, and this is no doubt what's causing the adjustments to be stored at the subexpressions rather than the outer paren expressions. I have to say this patch worries me slightly. In particular, I am not sure what other tables may be mis-indexed due to these changes, and it seems like this patch makes it more complex to know when the "irrelevant" parens can safely be skipped over and when they cannot. I'd feel better if the treatment of expr_paren was more uniform with other expressions (which is probably possible, given some refactoring, I don't quite know what problems you were running into that merited the introduction of each de-parenthesization). In general, I think it'd be a good idea to construct an "overlay CFG" for the AST that we use for all of our analysis (particularly flow-based things like liveness and (hopefully someday) borrow check, but also trans). I can elaborate more in some other forum on this idea, but it would have the benefit of allowing us to include parentheses in the AST for pretty-printing, but remove them for all other purposes. We could also do other similar normalizations. |
|
One disadvantage to both of these approaches is that they are not particularly resilient to modifications to the tree. For example, pretty-printed output from syntax extensions might not parse if it fails to insert paren nodes or entries into this map. In that regard, the current approach of having the pretty printer deduce where explicit parens are required is better. I wonder if we can simplify the syntax in any way to make this easier. |
|
I'm working on an improved version of this patch. |
for better pretty-printing, as per rust-lang#1458
|
I just uploaded an improved version that doesn't "deparenthesize". The only thing that's still a little suspicious is the This passes tests locally and I just pushed to |
|
@graydon said r+. I'm just waiting for the try build to finish and then I'll commit this. (We agreed that @nikomatsakis has a point insofar as it's important to not break syntax extensions, but also agreed to commit this now on the grounds that it fixes test cases and doesn't make things any worse. We can revisit in the future.) |
|
Merged: 165ce1 |
…htriplett Remove `Ipv6Addr::is_unicast_site_local` Removes the unstable method `Ipv6Addr::is_unicast_site_local`, see also rust-lang#85604 where I have tried to summarize related discussion so far. Unicast site-local addresses (`fec0::/10`) were deprecated in [IETF RFC rust-lang#3879](https://datatracker.ietf.org/doc/html/rfc3879), see also [RFC rust-lang#4291 Section 2.5.7](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.7). Any new implementation must no longer support the special behaviour of site-local addresses. This is mentioned in the docs of `is_unicast_site_local` and already implemented in `is_unicast_global`, which considers addresses in `fec0::/10` to have global scope, thus overlapping with `is_unicast_site_local`. Given that RFC rust-lang#3879 was published in 2004, long before Rust existed, and it is specified that any new implementation must no longer support the special behaviour of site-local addresses, I don't see how a user would ever have a need for `is_unicast_site_local`. It is also confusing that currently both `is_unicast_site_local` and `is_unicast_global` can be `true` for an address, but an address can actually only have a single scope. The deprecating RFC mentions that Site-Local scope was confusing to work with and that the classification of an address as either Link-Local or Global better matches the mental model of users. There has been earlier discussion of removing `is_unicast_site_local` (rust-lang#60145 (comment)) which decided against it, but that had the incorrect assumption that the method was already stable; it is not. (This confusion arose from the placement of the unstable attribute on the entire module, instead of on individual methods, resolved in rust-lang#85672) r? `@joshtriplett` as reviewer of all the related PRs
…riplett Remove `Ipv6Addr::is_unicast_site_local` Removes the unstable method `Ipv6Addr::is_unicast_site_local`, see also rust-lang#85604 where I have tried to summarize related discussion so far. Unicast site-local addresses (`fec0::/10`) were deprecated in [IETF RFC rust-lang#3879](https://datatracker.ietf.org/doc/html/rfc3879), see also [RFC rust-lang#4291 Section 2.5.7](https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.7). Any new implementation must no longer support the special behaviour of site-local addresses. This is mentioned in the docs of `is_unicast_site_local` and already implemented in `is_unicast_global`, which considers addresses in `fec0::/10` to have global scope, thus overlapping with `is_unicast_site_local`. Given that RFC rust-lang#3879 was published in 2004, long before Rust existed, and it is specified that any new implementation must no longer support the special behaviour of site-local addresses, I don't see how a user would ever have a need for `is_unicast_site_local`. It is also confusing that currently both `is_unicast_site_local` and `is_unicast_global` can be `true` for an address, but an address can actually only have a single scope. The deprecating RFC mentions that Site-Local scope was confusing to work with and that the classification of an address as either Link-Local or Global better matches the mental model of users. There has been earlier discussion of removing `is_unicast_site_local` (rust-lang#60145 (comment)) which decided against it, but that had the incorrect assumption that the method was already stable; it is not. (This confusion arose from the placement of the unstable attribute on the entire module, instead of on individual methods, resolved in rust-lang#85672) r? `@joshtriplett` as reviewer of all the related PRs
This is a pull request to implement @graydon 's suggestion, way back in #1458, of preserving parenthesization in the AST.
I have the feeling that WRT trans and typeck, there could be fewer explicit special cases for
expr_paren. This has something to do with theadjustmentstable in the type context, and I don't entirely understand how it works. At any rate, this passes tests. Suggestions welcome.Unfortunately, the test from #1458 still doesn't work, for apparently-separate reasons.
r? @graydon