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

Issue 1458 #3879

Closed
wants to merge 2 commits into from
Closed

Issue 1458 #3879

wants to merge 2 commits into from

Conversation

catamorphism
Copy link
Contributor

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 the adjustments table 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

@catamorphism
Copy link
Contributor Author

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?"

@nikomatsakis
Copy link
Contributor

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.

@nikomatsakis
Copy link
Contributor

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.

@catamorphism
Copy link
Contributor Author

I'm working on an improved version of this patch.

@catamorphism
Copy link
Contributor Author

I just uploaded an improved version that doesn't "deparenthesize". The only thing that's still a little suspicious is the expr_paren case in check::check_expr_with_unifier. I'm happy to explain that if necessary.

This passes tests locally and I just pushed to try.

@catamorphism
Copy link
Contributor Author

@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.)

@catamorphism
Copy link
Contributor Author

Merged: 165ce1

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 16, 2021
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2021
…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
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants