Change location of where clause on GATs by jackh726 · Pull Request #90076 · rust...
source link: https://github.com/rust-lang/rust/pull/90076
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
This comment has been hidden.
This comment has been hidden.
The latest upstream changes (presumably #90188) made this pull request unmergeable. Please resolve the merge conflicts.
As discussed on our call, let's change this to store a usize "split point" instead.
Outdated Show resolved
This comment has been hidden.
This comment has been hidden.
One concern I have are generic constants, for example
const SIZE_OF<T>: usize = std::mem::size_of::<T>();
trait IsEqual: 'static {
const TO<U: 'static>: bool;
}
impl<T: 'static> IsEqual for T {
const TO<U: 'static>: bool = TypeId::of::<T>() == TypeId::of::<U>();
}while both of these are currently not allowed in Rust, I personally quite strongly believe that we should add them in the future and want to at least want to keep this option open. Can we unambiguously parse where X after an expression? I would assume yes but am not sure myself.
impl<T: 'static> IsEqual for T {
const TO<U>: bool = TypeId::of::<T>() == TypeId::of::<U>()
where
U: 'static;
}@rfcbot fcp cancel
This shouldn't have been a T-compiler FCP!
This comment has been hidden.
One concern I have are generic constants, for example
const SIZE_OF<T>: usize = std::mem::size_of::<T>(); trait IsEqual: 'static { const TO<U: 'static>: bool; } impl<T: 'static> IsEqual for T { const TO<U: 'static>: bool = TypeId::of::<T>() == TypeId::of::<U>(); }while both of these are currently not allowed in Rust, I personally quite strongly believe that we should add them in the future and want to at least want to keep this option open. Can we unambiguously parse
where Xafter an expression? I would assume yes but am not sure myself.
Ah, good call! I'm glad you brought that up, we didn't discuss associated constants in the meeting. I'm not worried about parsing ambiguities, the where keyword is a pretty strong signal that we've reached the end of the expression (it's not presently used anywhere else in Rust that I know of).
I'm not 100% sure where I think where should go relative to the = though. It seems to depend a bit on the size of the constant expression.
EDIT: Upon further reflection, I think that I would probably expect where clauses to come after the value for const expressions, too, for the same reasons give above. I don't really expect "complex" const values that span many lines; though I don't doubt they will occur, I suspect it would also be better for folks to factor them out into a const fn.
I'm not 100% sure where I think
whereshould go relative to the=though. It seems to depend a bit on the size of the constant expression.
Do you remember the Centril's proposal to abbreviate fn foo() { result } to fn foo() = result;?
That is a very reasonable thing to do if the body is short, or the body is some kind of match or loop, and I'd still want to have it accepted one day.
(I can't find the relevant RFC PR/issue, unfortunately.)
fn foo() -> u8 = match x {
A => 1,
B => 2,
C => 3,
_ => 999,
};What is the preferable position for where in this case?
(Self-assigning, because I have some comments about the implementation as well.)
(I can't find the relevant RFC PR/issue, unfortunately.)
I believe this is it: Centril/rfcs#17
Something is wrong with the FCP here, it's still active and assigned to the compiler team.
@rfcbot fcp cancel
UPD: The previous cancel didn't work because nikomatsakis removed the T-compiler label before writing @rfcbot fcp cancel.
This comment has been hidden.
@rfcbot fcp cancel
@petrochenkov proposal cancelled.
@calebcartwright I might be missing something here but I don't think this change introduces a lot of complexity to rustfmt.
Thanks @nikomatsakis
It's more so about process as opposed to the specifics of the formatting/difficulty of implementation in rustfmt. My ask is simply that for now rustfmt not be modified to reformat aliases with the updated syntax (that can be accomplished by returning None on the relevant code paths) and that we have a chance for the formatting to first be discussed in the fmt-rfcs repo and then added to the Style Guide (I/rustfmt team are happy to handle those formatting-related activities)
As for my rationale:
Regardless of how simple and straightforward it may seem at first blush, there's (somewhat) edgier cases we have to account for which I don't think anyone but rustfmt necessarily cares about.
Examples of some cases with the old position where I'd have some questions around how they should be formatted in the new position:
impl<T> Iterable for Vec<T> {
// with new syntax lhs and rhs wouldn't fit on the same line
type SomeRatherAbsurdlyLongIdentBecauseReasonsAndFormattingFun<'a>
where
Self: 'a,
= <&'a [T] as IntoIterator>::IntoIter;
}
impl ... {
// what will we want to do with a type that can't fit on one line?
type TRef<'a>
where
<North as Foo>::ABCDEFGH: 'a,
<South as Foo>::ABCDEFGH: 'a,
<East as Foo>::ABCDEFGH: 'a,
<West as Foo>::ABCDEFGH: 'a,
<NorthEast as Foo>::ABCDEFGH: 'a,
= Any<
&'a North::ABCDEFGH,
&'a South::ABCDEFGH,
&'a East::ABCDEFGH,
&'a West::ABCDEFGH,
&'a NorthEast::ABCDEFGH,
>;
}
Those are solely illustrative, and admittedly heavily contrived, but hopefully paint a picture of completely plausible scenarios where due to idents, indentation levels, comments, whatever, rustfmt couldn't have a single line with the lhs and rhs preceding the clause in the new position. Additionally with new syntax we also have to consider interactions with config options.
This why I typically prefer that opinionated reformatting behavior not be incorporated with new syntax/syntax changes, and the approach I'm requesting here is the same tact that was taken for the new let-else statements. Once rustfmt starts reformatting constructs, any additional updates/changes have the potential to introduce breaking formatting updates which is something we strive to avoid.
More generally beyond some of the aforementioned TBDs, I want to make sure we don't put ourselves in a position where the Style Guide explicitly says clauses and aliases must be formatted like X, while in some cases rustfmt violates that and formats them like Y due to an as-of-yet unwritten set rules.
I also want to ensure everyone has a chance to weigh in on the formatting since we always communicate to users, especially those frustrated with rustfmt output, that formatting decisions in the Style Guide undergo a similar type RFC process in the fmt-rfcs repo.
I still think doing this is a mistake.
For the sake of a minor stylistic improvement this proposals starts multiplying entities and creating new issues and questions.
type Alias
where Long: WhereClause
= TypeValuse;Already works and looks fine when formatted even if the where clause is long, no extra action is required and people are free to occupy themselves with something more useful.
This part
the value of a type alias or associated type, is conceptually part of the "signature"
is highly unobvious and unintuitive, because a high-level semantic reasoning about interfaces and their uses is involved into making a syntactic choice. Syntactically signature is a "text to the left of = or {}".
With inconsistent rules like this nobody is going to remember where the where is written in each specific case.
The existing tuple struct case struct S<T, U>(T, U) where T: Trait; is unfortunate, but it was introduced due to a syntactic ambiguity caused by the parentheses, not due to the "end of signature" reasoning.
It wasn't widely discussed when introduced, because it's so old that almost no changes were widely discussed back then.
Even if we cannot remove inconsistencies like this, we should at least not introduce new ones.
@rfcbot concern further-thought
I'd like to give @petrochenkov's argument further thought, because it gives me serious pause and feels like compelling consistency, and I don't feel comfortable going forward with this right now.
My fingers and intuition also feel inclined towards where-after-RHS, but it no longer feels obvious to me if that's worth the inconsistency.
I discussed this some with @jackh726 today. He said he was feeling on the fence. I will say that I don't feel it's an open-and-shut case, though I still lean towards changing the syntax. Nonetheless, I had a few examples come up today which sparked different observations (some in favor of this FCP, some against).
First, take a look at this commit. I was struck by two things:
- I continue to feel that the
: Boundsand= Typeplay analogous roles in my brain, and I was surprised by the way that thewhere Self: 'aappeared at the end of thetypeitems in trait definitions but at the beginning of thetypeitems inimplitems. - On the other hand, I admit that simple cases like
type Cost<'a> where Self: 'a = MP::Cost<'a>;seem natural enough, and thattype Cost<'a> = MP::Cost<'a> where Self: 'a;might even confuse people about the scoping.
Moving on, [this playground] included a snippet with = sign formatted on the same starting point as where:
type Output<'a>
where
Self: 'a,
= <<T as Deref>::Target as GetOutput>::Output<'a>;Overall my reaction was "ugly but clear enough".
No conclusions, just observations. I do think the best thing at this stage would be for folks to try writing code that uses GATs and reformatting it to the distinct styles.
It's a bit of a drag that we can't have the compiler parse both and try to experience them both "as they would be while editing".
Sorry if I'm not caught up here (I tried to do as much reading as I could before responding, but I may have missed the answer to this question). What is the justification for allowing where clauses to be written at all when the trait is being implemented (and the type "assigned" or whatever you wanna call it)? Based on my understanding of GATs, implementers of the trait may not strengthen or weaken the where clauses listed in the trait definition. If I am not mistaken, this would then be unique to Rust in that it is the only place in which where clauses are allowed to be written, but are effectively not allowed to have any meaning.
Thank you! To be honest, I'm not terribly convinced by that argument, but this being unnecessarily irreversible is a good point and I am happy to see that this is being acknowledged as a direction in which we might head in the future.
@joshtriplett and I had a conversation a few days back and he indicated that he was happy so long as we have good diagnostics for both variants, in short, with an actionable rustfix. I also feel (and he did not disagree) that there ought to be one "officially preferred" location. I further feel (and he did not disagree) that the preferred location ought to be after the =, for the reasons I've previously stated.
That said, we were debating between two variants:
- accept both variants and concatenate them, but issue a lint warning (with rustfix) to rewrite where clauses before the
=into where clauses after the= - produce a hard error for lint warnings before the
=with actionable rustfix
To be honest, I somewhat lean towards the first. I don't like "more than one way to do it for no particularly good reason", but the lint warning addresses that concern. Furthermore, parsing both variants means whichever people more naturally do it, the code will compile. The fact that both are legal Rust also means we can have rustfmt silently normalize the non-deprecated variant. If there were some particular use case (e.g. with macros) where it's more convenient for the where clause to come before the =, that is still possible. It's hard for me to see the downside in practical terms.
Therefore, I'm curious to hear what folks thing about that idea:
Accept both and concatenate, but have "before the =" be deprecated.
I think that's a pretty decent plan.
There's two benefits to this approach:
- Any existing code with GATs will continue to compile. Yes, it's an unstable feature, but it would be nice to not make it a chore to update from a current nightly.
- It would give us real data for which variant people prefer (with a bias for after the equals, of course). This could be used to later inform hard-comitting to one or the other at an edition.
The fact that both are legal Rust also means we can have rustfmt silently normalize the non-deprecated variant.
If this is indeed the variant/route selected, would folks be open to doing this in a phased approach wherein for some time rustfmt would continue to format clauses found in the existing position as dictated by the style guide + leaving clauses in the new position alone, and at some future date transition to performing the transformation to the new position once we've had time to codify all the rules (including the fun edge cases)?
@calebcartwright I would be fine with that. We would issue a rustfix regardless I think.
@joshtriplett Is there any further concern for the FCP? Or can that be resolved?
This comment has been hidden.
No further concern, assuming we parse both and either lint or hard error on one with a rustfix towards the other.
@rfcbot resolve further-thought
Copy link
rfcbot commented 5 days ago
This is now entering its final comment period, as per the review above.
The latest upstream changes (presumably #93069) made this pull request unmergeable. Please resolve the merge conflicts.
removing T-compiler label which I have probably mistakenly added (by reading this comment)
@rustbot label -T-compiler
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK