Stream: t-compiler

Topic: ┬┐revert? PR Pre-expansion gate most of the things #65742


pnkfelix (Oct 31 2019 at 15:02, on Zulip):

fresh topic

nikomatsakis (Oct 31 2019 at 15:02, on Zulip):

Some background context from weekly meeting thread

not sure how much background is worth going into here. This has to do with a change to feature gate syntax like box 22, even if that occurs in a #[cfg] syntax. This affects "stable" code that used such features conditionally -- the correct way to do so after this PR is to put them in a separate file and use a #[cfg(foo)] mod m; statement

nikomatsakis (Oct 31 2019 at 15:03, on Zulip):

But, as we said, this is only partly about the specifics, and more a question of "general policy" --

pnkfelix (Oct 31 2019 at 15:03, on Zulip):

right; one relevant issue (with a significant comment thread) is at #65860

nikomatsakis (Oct 31 2019 at 15:03, on Zulip):

We don't really know what add'l impact there may be, correct?

pnkfelix (Oct 31 2019 at 15:04, on Zulip):

On the one hand, we don't tend to have a knee-jerk "oh a single user identified a regression; we must revert"

nikomatsakis (Oct 31 2019 at 15:04, on Zulip):

That said, can I get a complete picture? I believe this PR is currently in nightly, not yet beta, right?

simulacrum (Oct 31 2019 at 15:04, on Zulip):

We can get a complete(er) picture, but it would take time -- a crater run and a few try builds

pnkfelix (Oct 31 2019 at 15:04, on Zulip):

the PR landed 8 days ago, so yes, I believe its currently in nightly only

pnkfelix (Oct 31 2019 at 15:05, on Zulip):

I suppose that can be something that affects the revert calculation

simulacrum (Oct 31 2019 at 15:05, on Zulip):

To be clear my primary argument for reverting is _not_ the regression. It is the fact that I expect an RFC (or design meeting or something along those lines) for this change -- at least, some discussion.

nikomatsakis (Oct 31 2019 at 15:06, on Zulip):

I'm trying to remember if/where this change was discussed. It may have been, I'm not sure (which is perhaps part of the problem)

nikomatsakis (Oct 31 2019 at 15:06, on Zulip):

There are definitely hard technical constraints that may impede other solutions

pnkfelix (Oct 31 2019 at 15:07, on Zulip):

To be clear my primary argument for reverting is _not_ the regression. It is the fact that I expect an RFC (or design meeting or something along those lines) for this change -- at least, some discussion.

but what is the criteria you are using, besides the regression, to determine that there should have been a design meeting or discussion?

pnkfelix (Oct 31 2019 at 15:07, on Zulip):

that is, I think it is the coupling of the two factors that leads one to consider reverting the change

centril (Oct 31 2019 at 15:07, on Zulip):

I would expect an RFC to not pre-expansion gate --- taking away pre-expansion gating is easy; putting it back for (at least new stuff) is hard

nikomatsakis (Oct 31 2019 at 15:08, on Zulip):

It seems clear that we need some solution

nikomatsakis (Oct 31 2019 at 15:08, on Zulip):

But the point is that an RFC would discuss what solution we want, and document why we want it

simulacrum (Oct 31 2019 at 15:08, on Zulip):

I agree that we need some solution. I feel like there are multiple options. This is a breaking change (even if allowed by a strict interpretation of our stability rules).

pnkfelix (Oct 31 2019 at 15:09, on Zulip):

I would expect an RFC to not pre-expansion gate --- taking away pre-expansion gating is easy; putting it back for (at least new stuff) is hard

What is the reasoning here? Is the argument that the prior behavior was an accidental implementation artifact?

nikomatsakis (Oct 31 2019 at 15:09, on Zulip):

On the other hand, it is clear that we make changes of this magnitude without RFCs -- though I'm not sure that's good

centril (Oct 31 2019 at 15:09, on Zulip):

@pnkfelix yes, and we are pre-expansion gating all new features

simulacrum (Oct 31 2019 at 15:09, on Zulip):

So to be clear I don't really care too much if this is a full fledged RFC, but at least some discussion and summary of alternatives would be good IMO

simulacrum (Oct 31 2019 at 15:10, on Zulip):

AFAIK, that discussion has not happened (or, at least, it was not had in sufficient publicity)

nikomatsakis (Oct 31 2019 at 15:10, on Zulip):

What is the reasoning here? Is the argument that the prior behavior was an accidental implementation artifact?

I do think this is "true" -- of course, one that dates way back. But it's not like we intended to stabilize unstable syntax. :) We made efforts to avoid that with macros, which were only partly successfully, and overlooked this similar case.

pnkfelix (Oct 31 2019 at 15:11, on Zulip):

true, and we basically decided we couldn't afford to fix macro_rules! 100%

centril (Oct 31 2019 at 15:11, on Zulip):

@nikomatsakis there are for example comments of yours in 2018 that we should have done pre-expansion gating and that we should do it on new stuff

nikomatsakis (Oct 31 2019 at 15:11, on Zulip):

At the same time, I don't see how that makes RFCs (or other forms of discussion documents) inapplicable

pnkfelix (Oct 31 2019 at 15:12, on Zulip):

can I get a clarification about "new stuff" ?

pnkfelix (Oct 31 2019 at 15:12, on Zulip):

because I am getting impression that this change has affected old stuff

simulacrum (Oct 31 2019 at 15:12, on Zulip):

e.g. or patterns I believe

simulacrum (Oct 31 2019 at 15:12, on Zulip):

those were gated upon landing

centril (Oct 31 2019 at 15:12, on Zulip):

@pnkfelix new active feature gates; everything new for some months has been pre-expansion gated

pnkfelix (Oct 31 2019 at 15:12, on Zulip):

but maybe I misunderstand @RalfJ 's complaint?

nikomatsakis (Oct 31 2019 at 15:12, on Zulip):

I think that's the change here -- it was extended to old stuff.

simulacrum (Oct 31 2019 at 15:12, on Zulip):

this specific issue is about gating old (but still unstable) syntax, e.g. box patterns, type ascription

pnkfelix (Oct 31 2019 at 15:13, on Zulip):

okay, so using a better technology to gate new stuff sounds great and its what we're doing and should be fine

centril (Oct 31 2019 at 15:13, on Zulip):

yes, based on crater -- we didn't gate some of the old stuff where the regressions were too high

pnkfelix (Oct 31 2019 at 15:13, on Zulip):

and the question is then about when/if to extend the new gating system to apply it to the old stuff

pnkfelix (Oct 31 2019 at 15:13, on Zulip):

Is that correct so far?

simulacrum (Oct 31 2019 at 15:13, on Zulip):

My problem is that the specific form of gating seems not-perfect, globally

simulacrum (Oct 31 2019 at 15:13, on Zulip):

not just for new stuff, but also for old things

nikomatsakis (Oct 31 2019 at 15:13, on Zulip):

yes, based on crater -- we didn't gate some of the old stuff where the regressions were too high

this is good, I was not aware of this

centril (Oct 31 2019 at 15:14, on Zulip):

@pnkfelix I think there's a point of view here that new stuff shouldn't be pre-expansion feature gated either

nikomatsakis (Oct 31 2019 at 15:14, on Zulip):

mm I don't think so, but I think there's a view that we can find a better fix than the current one

pnkfelix (Oct 31 2019 at 15:14, on Zulip):

because the inline attribute #![cfg_blah_whatever(..)] coding pattern just stops working for that new stuff ?

centril (Oct 31 2019 at 15:15, on Zulip):

@pnkfelix right; well -- Vadim's delay_parsing! macro sorta hacks back an inline approach

centril (Oct 31 2019 at 15:15, on Zulip):

@nikomatsakis I know that is not @simulacrum's view, I think it's Ralf's view

centril (Oct 31 2019 at 15:15, on Zulip):

or at least that's the sense I get

simulacrum (Oct 31 2019 at 15:15, on Zulip):

That is not my sense of Ralf's view.

nikomatsakis (Oct 31 2019 at 15:15, on Zulip):

I mean I don't think Ralf is saying "we should implicitly stabilize new unstable syntax"

nikomatsakis (Oct 31 2019 at 15:16, on Zulip):

but I think Ralf is saying that the current fix is crude

nikomatsakis (Oct 31 2019 at 15:16, on Zulip):

and that having to isolate unstable syntax into separate files is surprising and a pain, and a breaking change in practice

centril (Oct 31 2019 at 15:16, on Zulip):

well again; delay_parse! :slight_smile:

centril (Oct 31 2019 at 15:17, on Zulip):

put it in the standard library, and you even have a "documented solution with all the explanation of why"

centril (Oct 31 2019 at 15:17, on Zulip):

(of course, that's for t-libs to decide...)

simulacrum (Oct 31 2019 at 15:17, on Zulip):

I think, for me, it comes down to there being alternative implementation strategies (with other downsides, of course, doesn't seem like there is a 100% win solution, or we don't know about it) -- but we have not explored those, and have not discussed them

simulacrum (Oct 31 2019 at 15:17, on Zulip):

or at least I have not seen evidence of exploration or discussion

nikomatsakis (Oct 31 2019 at 15:18, on Zulip):

I think there has probably been discussion but it's been spread out in time and informal

centril (Oct 31 2019 at 15:18, on Zulip):

@simulacrum I actually looked at the code for the tt idea -- it looked... complicated to me

simulacrum (Oct 31 2019 at 15:18, on Zulip):

I think there has probably been discussion but it's been spread out in time and informal

I would at least like a summary -- this is sort of what RFCs are for :)

nikomatsakis (Oct 31 2019 at 15:19, on Zulip):

e.g. it is good to know that crater was used to guide the set of old syntaxes we could gate; in an ideal world we'd have a documented plan saying that we are doing this, with a checklist, etc -- kind of like with NLL

simulacrum (Oct 31 2019 at 15:19, on Zulip):

but, regardless, even if the alternatives have downsides that are too big for us to allow, it does seem... like we should know that

centril (Oct 31 2019 at 15:19, on Zulip):

and I also trust Petrochenkov's judgement on libsyntax a whole lot on this, and it jives with my own

pnkfelix (Oct 31 2019 at 15:20, on Zulip):

/me needs to step away from keyboard for a while; I will try to review the thread later.

nikomatsakis (Oct 31 2019 at 15:20, on Zulip):

So let's step back a second, I think that getting into the altneratives in detail in this thread is beside the point

simulacrum (Oct 31 2019 at 15:20, on Zulip):

right, yeah

centril (Oct 31 2019 at 15:20, on Zulip):

Having https://github.com/rust-lang/rust/pull/65974 also means that one entire class of "crude"-ness goes away

centril (Oct 31 2019 at 15:21, on Zulip):

(it's basically only #[cfg] and the transcriber side at this point)

nikomatsakis (Oct 31 2019 at 15:22, on Zulip):

I think what @simulacrum feels is that this change should get broader discussion and documentation -- I'm not 100% sure if that requires backing out.

simulacrum (Oct 31 2019 at 15:22, on Zulip):

I'm not sure -- to me, it feels like we should not be landing changes which break code without a discussion and summary of alternatives. Whether that happens informally or not, I don't care too much, although I think I would prefer an RFC.

nikomatsakis (Oct 31 2019 at 15:22, on Zulip):

it sort of backs up the point @centril in its own way, as I don't really understand what that PR is doing :)

nikomatsakis (Oct 31 2019 at 15:22, on Zulip):

was there an FCP here?

centril (Oct 31 2019 at 15:23, on Zulip):

@nikomatsakis pretty easy to explain: if a macro-matcher fails, throw away all the pre-expansion gating done in it

nikomatsakis (Oct 31 2019 at 15:23, on Zulip):

we obviously do land name resolution changes often that are kind of "just PRs", but there is usually lang team check-off -- even if I think none of us are super happy with the amount of documentation and formality there

nikomatsakis (Oct 31 2019 at 15:23, on Zulip):

(to be clear, not blaming @Vadim Petrochenkov at all there! so happy they do what they do :)

centril (Oct 31 2019 at 15:25, on Zulip):

FCPs for confirming that we want to fix bugs seems strange in general -- it should take the agreement of the whole team not to fix them, not the agreement of the team to fix them

centril (Oct 31 2019 at 15:25, on Zulip):

otherwise we have bad defaults in terms of soundness and whatnot

nikomatsakis (Oct 31 2019 at 15:25, on Zulip):

@centril this is not about "fixing bugs" or not, it's about practical impact

nikomatsakis (Oct 31 2019 at 15:25, on Zulip):

like I think we all agree we want to fix this bug

centril (Oct 31 2019 at 15:26, on Zulip):

@nikomatsakis well let's focus on the practical impact; let's revert as needed based on data, going back to @pnkfelix's point about knee-jerk reactions

centril (Oct 31 2019 at 15:26, on Zulip):

and let's revert specific gates as needed

centril (Oct 31 2019 at 15:27, on Zulip):

and again, let the reviewer judge the practical impact

nikomatsakis (Oct 31 2019 at 15:28, on Zulip):

I'm trying to decide my reaction here:)

centril (Oct 31 2019 at 15:28, on Zulip):

take your time :P

nikomatsakis (Oct 31 2019 at 15:30, on Zulip):

take a look at the bug fix procedure

nikomatsakis (Oct 31 2019 at 15:30, on Zulip):

it mentions some specifics

nikomatsakis (Oct 31 2019 at 15:30, on Zulip):

for example:

Every breaking change should be accompanied by a dedicated tracking issue for that change. The main text of this issue should describe the change being made, with a focus on what users must do to fix their code. The issue should be approachable and practical; it may make sense to direct users to an RFC or some other issue for the full details. The issue also serves as a place where users can comment with questions or other concerns.

nikomatsakis (Oct 31 2019 at 15:31, on Zulip):

it also says things like:

Changes that are believed to have negligible impact can go directly to issuing an error.

and advises breaking up changes into smaller parts (which I believe is the strategy being used here)

centril (Oct 31 2019 at 15:31, on Zulip):

This procedure is what we use when we file C-future-compatibility issues that are linked to inside the compiler

nikomatsakis (Oct 31 2019 at 15:31, on Zulip):

this procedure is what we use when we fix compiler bugs that affect existing code

nikomatsakis (Oct 31 2019 at 15:32, on Zulip):

it has nothing to do with C-xxx labels?

nikomatsakis (Oct 31 2019 at 15:32, on Zulip):

From time to time, we encounter the need to make a bug fix, soundness correction, or other change in the compiler which will cause existing code to stop compiling.

nikomatsakis (Oct 31 2019 at 15:32, on Zulip):

we may not be perfectly consistent, but I think it expresses the values we are shooting for

nikomatsakis (Oct 31 2019 at 15:32, on Zulip):

anyway, the point that @simulacrum raised is more about how the decision to go forward was made in the first place

centril (Oct 31 2019 at 15:32, on Zulip):

huh; it describes the procedure for C-future-compatibility

nikomatsakis (Oct 31 2019 at 15:33, on Zulip):

I mean I don't even know that that label means

nikomatsakis (Oct 31 2019 at 15:33, on Zulip):

:)

simulacrum (Oct 31 2019 at 15:33, on Zulip):

it describes the procedure for all bugs, indicating that if we do experience practical changes to end user code, we should likely file a tracking issue (which would indeed be tagged as C-future-compat)

nikomatsakis (Oct 31 2019 at 15:34, on Zulip):

I think this is how I feel: I think this change has been made in a reasonable and responsible way overall, but corners were cut. Ther'es nothing implicitly wrong with that. We do it a lot, when we judge things will work out well, but it's always a kind of risk that somebody feels surprised or the impact is greater. In such cases, I think it is good for us to back rapidly off and "re-approach" -- even if the end result will be the same.

centril (Oct 31 2019 at 15:34, on Zulip):

Making an issue for soft-gating pre-expansion and hard-gating post-expansion will be quite hard technically; and without doing that we don't actually have an issue to point to people in warning message; that seems rather useless -- no one is going to find said issue

nikomatsakis (Oct 31 2019 at 15:34, on Zulip):

I also feel pushing hard back in response to complaints kind of pushes towards needing more process overall

nikomatsakis (Oct 31 2019 at 15:35, on Zulip):

Because part of how we can get by with less process is by responding kindly to objections

nikomatsakis (Oct 31 2019 at 15:36, on Zulip):

All of that said, I don't know that we need a full RFC here -- but it would be appropriate I think to document the plan, what it means for end-users -- I think @simulacrum is correct that the potential, complete impact of this change is large

nikomatsakis (Oct 31 2019 at 15:36, on Zulip):

we probably should've done that way back

nikomatsakis (Oct 31 2019 at 15:37, on Zulip):

but basically there's a "best practice" here for releasing crates that mix stable and unstable syntax, right?

nikomatsakis (Oct 31 2019 at 15:37, on Zulip):

(using modues)

centril (Oct 31 2019 at 15:37, on Zulip):

yes, or delay_parse! if one objects to having several files

nikomatsakis (Oct 31 2019 at 15:37, on Zulip):

that is a solution that has a lot going for it, given the internal architecture of the compiler, but basically the core decision here was to adopt that as the way forward, and then phase it in gradually

simulacrum (Oct 31 2019 at 15:37, on Zulip):

well, so that best practice is what @centril has been suggesting -- I personally would find it deeply wrong to make that a best practice

simulacrum (Oct 31 2019 at 15:38, on Zulip):

(or at least am unconvinced at this point that we should recommend it)

nikomatsakis (Oct 31 2019 at 15:38, on Zulip):

that's the point I think :) we've implicitly adopted that, and it has big medium implications, but there wasn't much conversation about it. Sometimes we get away with that, but sometimes we don't.

eddyb (Oct 31 2019 at 15:38, on Zulip):

one alternative is allowing more in #[cfg]-out code, i.e. not properly parsing the contents (anything nested in (...), [...], or {...})

nikomatsakis (Oct 31 2019 at 15:39, on Zulip):

(yes, this is what I was trying to say on thread about $tt)

centril (Oct 31 2019 at 15:39, on Zulip):

@eddyb @nikomatsakis y'all will need to explain that to me; are you talking about laziness on the lexer level?

nikomatsakis (Oct 31 2019 at 15:39, on Zulip):

(hold on, let's not lose the focus :)

simulacrum (Oct 31 2019 at 15:40, on Zulip):

(Yes, I think we know that there are at least possible alternatives, we should not discuss them just now)

eddyb (Oct 31 2019 at 15:40, on Zulip):

@centril token trees are properly nested, so you can lex but not parse certain things if you don't want to

centril (Oct 31 2019 at 15:40, on Zulip):

@eddyb that also means we cannot look-ahead into { blocks, no?

eddyb (Oct 31 2019 at 15:41, on Zulip):

anyway if fallout wasn't properly investigated in the PR, that's reason enough for reverting and reconsidering

simulacrum (Oct 31 2019 at 15:41, on Zulip):

Can we not discuss specific alternatives please? I don't want to get derailed :)

eddyb (Oct 31 2019 at 15:42, on Zulip):

I was led to believe, in the issue comments, that crater was used and the results were favorable, but that appears to not be the case?

centril (Oct 31 2019 at 15:42, on Zulip):

that is the case.

nikomatsakis (Oct 31 2019 at 15:42, on Zulip):

anyway if fallout wasn't properly investigated in the PR, that's reason enough for reverting and reconsidering

one thing I do think is that we should use an expedited process to "reconsider"

nikomatsakis (Oct 31 2019 at 15:42, on Zulip):

e.g., schedule a specific meeting to go over the alternatives and their pros/cons

centril (Oct 31 2019 at 15:42, on Zulip):

And reverting makes it harder to e.g. land https://github.com/rust-lang/rust/pull/65974

eddyb (Oct 31 2019 at 15:43, on Zulip):

we could steal tomorrow's slot, but that might not be a good idea idk

simulacrum (Oct 31 2019 at 15:43, on Zulip):

I am fine with that, and am willing to try and make time to help out with such an effort...

nikomatsakis (Oct 31 2019 at 15:43, on Zulip):

we could steal tomorrow's slot, but that might not be a good idea idk

I'd probably say just schedule a one-off thing next week?

simulacrum (Oct 31 2019 at 15:43, on Zulip):

I think we need a bit of prep work which would not be ready for tomorrow (but maybe next, or next next Friday)

nikomatsakis (Oct 31 2019 at 15:43, on Zulip):

I don't want to pre-empt tomorrow's discussion, which has been a long time coming too

nikomatsakis (Oct 31 2019 at 15:43, on Zulip):

Although I forget what we're discussing next week, that could maybe work

nikomatsakis (Oct 31 2019 at 15:43, on Zulip):

@centril is there a "lightway" way to revert?

centril (Oct 31 2019 at 15:43, on Zulip):

well, so that best practice is what @centril has been suggesting -- I personally would find it deeply wrong to make that a best practice

@simulacrum It sounds like you are saying that pre-expansion gating shouldn't happen for new stuff either until we make changes

eddyb (Oct 31 2019 at 15:44, on Zulip):

if we revert, I prefer we do so before beta branches, so we have a full cycle to test out whatever ends up landing

nikomatsakis (Oct 31 2019 at 15:44, on Zulip):

(I didn't look at the PR, but I don't know if there's a way to keep most of the code intact but just disable some specific cases)

nikomatsakis (Oct 31 2019 at 15:44, on Zulip):

if we revert, I prefer we do so before beta branches, so we have a full cycle to test out whatever ends up landing

this is another point

nikomatsakis (Oct 31 2019 at 15:44, on Zulip):

I feel like the ideal time for this change to land would be just after beta branches :)

eddyb (Oct 31 2019 at 15:45, on Zulip):

we could maybe do a weird revert where we add the old logic back but don't delete the duplicate one that runs earlier

simulacrum (Oct 31 2019 at 15:45, on Zulip):

This is why I've been pushing somewhat hard on this, I would like to land early in the cycle if we do land.

centril (Oct 31 2019 at 15:45, on Zulip):

@nikomatsakis the pre-expansion PR consists of moving individual PRs to pre-expansion gating in different commits

eddyb (Oct 31 2019 at 15:45, on Zulip):

and either silence, or turn into warnings, the new earlier errors

centril (Oct 31 2019 at 15:45, on Zulip):

(oh also, it fixes a syntax bug)

centril (Oct 31 2019 at 15:46, on Zulip):

I think we should do beta crater runs and backport fixes as needed

centril (Oct 31 2019 at 15:46, on Zulip):

that shouldn't be too hard and I can commit to doing so myself

simulacrum (Oct 31 2019 at 15:46, on Zulip):

@centril, could you elaborate more on why you're against trying to proceed more carefully here?

eddyb (Oct 31 2019 at 15:46, on Zulip):

@nikomatsakis if you like my plan I could spend a few hours trying it out

centril (Oct 31 2019 at 15:46, on Zulip):

@simulacrum idk why this is proceeding less carefully?

simulacrum (Oct 31 2019 at 15:47, on Zulip):

....it seems clear that keeping the change in is less careful than backing it out and discussing before putting it back in?

centril (Oct 31 2019 at 15:47, on Zulip):

The end result seems the same: if we find too high regression counts on beta we revert those gates

nikomatsakis (Oct 31 2019 at 15:49, on Zulip):

it seems clear that we prefer not to muck with beta

centril (Oct 31 2019 at 15:49, on Zulip):

@simulacrum after the matcher PR lands, we could do a "beta" crater run

simulacrum (Oct 31 2019 at 15:49, on Zulip):

I think it's not about the destination, it's about the path. I would definitely find it less stressful to back this out and not need to worry about tracking bugfixes into beta, etc

Pietro Albini (Oct 31 2019 at 15:50, on Zulip):

(and here I thought setting up zulip notifications every time someone said "crater" was a good idea :sweat_smile:)

eddyb (Oct 31 2019 at 15:50, on Zulip):

the pre-expansion gating reaching beta would be a headache to deal with

eddyb (Oct 31 2019 at 15:50, on Zulip):

@Pietro Albini wow you can do that?!

nikomatsakis (Oct 31 2019 at 15:50, on Zulip):

(I just discovered that feature recently myself)

nikomatsakis (Oct 31 2019 at 15:50, on Zulip):

I think it's not about the destination, it's about the path. I would definitely find it less stressful to back this out and not need to worry about tracking bugfixes into beta, etc

well put

centril (Oct 31 2019 at 15:50, on Zulip):

@eddyb huh? these are trivial reverts to make

nikomatsakis (Oct 31 2019 at 15:51, on Zulip):

If they are trivial, then why not do them now?

nikomatsakis (Oct 31 2019 at 15:51, on Zulip):

I feel like we're all working pretty hard to be reasonable here, quite frankly, and it's frustrating to me that you're not listening.

centril (Oct 31 2019 at 15:51, on Zulip):

in fact, we need beta crater runs to check if the previous crater run is wrong

eddyb (Oct 31 2019 at 15:51, on Zulip):

just compile the compiler as non-nightly?

eddyb (Oct 31 2019 at 15:51, on Zulip):

you don't need it to be a real beta, you can do this with a try build

eddyb (Oct 31 2019 at 15:52, on Zulip):

it's... somewhere in the codebase, I can look for it if you want

eddyb (Oct 31 2019 at 15:52, on Zulip):

(assuming you want #![feature] to be disabled)

centril (Oct 31 2019 at 15:53, on Zulip):

@nikomatsakis from my perspective it feels like this: we did a crater run; I spent a whole lot of time investigating @simulacrums report, a PR was filed to fix the matcher issue -- I think some of the data exists, so why are we ignoring the crater run?

nikomatsakis (Oct 31 2019 at 15:53, on Zulip):

Fair. I think the answer is that most of us didn't really understand the full implications, and we'd like to review the options.

nikomatsakis (Oct 31 2019 at 15:54, on Zulip):

(I do think a beta crater run is a good idea though! I'd prefer to have that data on hand when having the discussion.)

nikomatsakis (Oct 31 2019 at 15:54, on Zulip):

That plus:

nikomatsakis (Oct 31 2019 at 15:54, on Zulip):

It's better to make this change as early in cycle as possible regardless

nikomatsakis (Oct 31 2019 at 15:54, on Zulip):

I feel like we're quite likely going to do this change -- and I am not even trying to imply you handled it irresponsibly, quite the opposite.

nikomatsakis (Oct 31 2019 at 15:55, on Zulip):

(Though I do think there are viable alternatives)

nikomatsakis (Oct 31 2019 at 15:55, on Zulip):

But some of us at least find having things on beta that we don't plan to ship stressful :)

centril (Oct 31 2019 at 15:55, on Zulip):

(I think those viable alternatives need to be presented as PRs to be believable)

nikomatsakis (Oct 31 2019 at 15:56, on Zulip):

I don't think that's a reasonable attitude.

simulacrum (Oct 31 2019 at 15:56, on Zulip):

I agree with that as well, I think the process here was fine, but now that we have a few people who have -- IMO reasonable, but I am somewhat biased -- concerns, we should tend towards proceeding carefully, which to me means reverting the PR and waiting on that discussion to happen.

centril (Oct 31 2019 at 15:56, on Zulip):

@simulacrum are you objecting to pre-expansion gating new things?

nikomatsakis (Oct 31 2019 at 15:56, on Zulip):

I don't think that's a reasonable attitude.

to be clear, I don't mean that impl concerns don't matter, I just mean the fact that we don't have any PRs yet isn't evidence of anything.

eddyb (Oct 31 2019 at 15:56, on Zulip):

I already have two things I'm tangentially involved in to worry about on beta (promotion/const-checking changes and caller_location), a third is too much stress, to the point where I'd rather help out on this just for the peace of mind

centril (Oct 31 2019 at 15:57, on Zulip):

@nikomatsakis I'm just looking at the actual code and matching that up with the tt idea and it does not seem realistic to me

nikomatsakis (Oct 31 2019 at 15:57, on Zulip):

@centril I don't think anyone is proposing to back that out -- though I think that if we decide to take another approach, it would apply equally to new syntax.

nikomatsakis (Oct 31 2019 at 15:57, on Zulip):

but new syntax has no users yet

nikomatsakis (Oct 31 2019 at 15:57, on Zulip):

so if we pre-expansion gate it while adding, that seems fine

nikomatsakis (Oct 31 2019 at 15:57, on Zulip):

(and of course in the fullness of time, we should use the same approach for all unstable syntax)

centril (Oct 31 2019 at 15:58, on Zulip):

@simulacrum does ^-- reflect your view?

centril (Oct 31 2019 at 15:58, on Zulip):

seems so :slight_smile:

simulacrum (Oct 31 2019 at 15:58, on Zulip):

I think, yes. But I will say that I have not had the time to fully think about this... this is at least part of why I would like for us to proceed less quickly

centril (Oct 31 2019 at 16:00, on Zulip):

@simulacrum if you want to revert the PR (partially; the syntax bug needs to be fixed), that's fair; I will rebase my matcher PR to remove the newer pre-expansion gates -- can you run the "beta" crater stuff?

simulacrum (Oct 31 2019 at 16:02, on Zulip):

I am unclear as to exactly what I am agreeing to. I can help run crater on some PR in a beta-esque fashion, but I do not know if I have the ability/time to dig into the code and figure out exactly what we should be running crater on.

I am also unclear -- are you on board with a revert?

centril (Oct 31 2019 at 16:03, on Zulip):

@simulacrum my understanding is that we would test the same PR with crater but without it being a nightly compiler

centril (Oct 31 2019 at 16:04, on Zulip):

I am also unclear -- are you on board with a revert?

Yes, but like everyone else, I have other PRs and work I need to do

Wesley Wiser (Oct 31 2019 at 16:04, on Zulip):

I think being cautious here and reverting the change (for now) helps users trust the stability of Rust because they can see that even if we're allowed to fix something that causes small breakages, we'll do so carefully. That trust is really valuable especially if (when) we need to make backwards-incompatible changes to resolve issues which affect larger parts of the ecosystem.

nikomatsakis (Oct 31 2019 at 16:04, on Zulip):

(Yes, I think this is important too.)

simulacrum (Oct 31 2019 at 16:05, on Zulip):

Okay -- I can put myself on the hook to put up a full revert, but since I don't know about the syntax bug you're referring to, I would need to dig in to the specific PR and try to figure things out.

simulacrum (Oct 31 2019 at 16:05, on Zulip):

I don't think I have the time to do that.

eddyb (Oct 31 2019 at 16:05, on Zulip):

@centril like I said, I can spend a few hours on a straight-forward revert, or one that doesn't kill the new code entirely (we could put it behind a feature-flag, turn it into an error, etc.)

eddyb (Oct 31 2019 at 16:05, on Zulip):

I just need a confirmation that we're doing this

centril (Oct 31 2019 at 16:06, on Zulip):

@simulacrum https://github.com/rust-lang/rust/pull/65742/commits/2e64bb2d37d5f15113e0a7199cd684504c6b8de7 needs to stay

centril (Oct 31 2019 at 16:06, on Zulip):

@eddyb are you talking about soft-gating the pre-expansion?

nikomatsakis (Oct 31 2019 at 16:08, on Zulip):

OK, I have to go, I've skipped a meeting already and have to catch up with that now :)

eddyb (Oct 31 2019 at 16:08, on Zulip):

maybe? not sure what "soft" means in this context. I am mostly talking about keeping the new code around (just not with the errors it produces today) if that's desireable

eddyb (Oct 31 2019 at 16:08, on Zulip):

tests, too

eddyb (Oct 31 2019 at 16:08, on Zulip):

@nikomatsakis got it :)

centril (Oct 31 2019 at 16:08, on Zulip):

I don't think that's useful

nikomatsakis (Oct 31 2019 at 16:08, on Zulip):

I'm good with anything that stops the errors, I think a "less disruptive" PR seems fine, as long as it's not too much work

centril (Oct 31 2019 at 16:09, on Zulip):

@nikomatsakis soft means warnings for pre-expansion

centril (Oct 31 2019 at 16:09, on Zulip):

oops; @eddyb ^

centril (Oct 31 2019 at 16:09, on Zulip):

+ you also need hard gating for post-expansion

centril (Oct 31 2019 at 16:09, on Zulip):

but that work would be premature

centril (Oct 31 2019 at 16:09, on Zulip):

brb for a bt

nikomatsakis (Oct 31 2019 at 16:12, on Zulip):

like a future-compatibility warning? I could live with that, though I think a straight revert would be easier -- the follow-up work would be delayed of course. If we do it, we'd need an issue documenting a bit what's going on etc. I guess it feels a bit like putting the cart before the horse.

nikomatsakis (Oct 31 2019 at 16:12, on Zulip):

but I wouldn't stand in the way

centril (Oct 31 2019 at 16:18, on Zulip):

@nikomatsakis a future compat warning isn't easy to do here

centril (Oct 31 2019 at 16:21, on Zulip):

well maybe not all that difficult; you mark spans with whether you are in expansion or not when parsing, then partition the set based on those; finally you warn/error based on those

centril (Oct 31 2019 at 16:22, on Zulip):

but it's a waste of effort; we should first check crater runs before adding hacks to the compiler

simulacrum (Oct 31 2019 at 16:22, on Zulip):

So to circle up a bit here -- it sounds like @eddyb is going to try and get a PR going that reverts this change in some loose form (maybe fully, or perhaps in a softer way). Then we should try and pencil in a meeting next week to talk about this change and get a path forward, before which we would like to have a document with a beta-esque crater regression report (that has been triaged) and some document summarizing the current PR's rationale and decisions made and possible alternatives to those decisions.

simulacrum (Oct 31 2019 at 16:23, on Zulip):

concretely I don't think that work can happen in time for next week, so I would propose we try and schedule for two weeks from nowish.

centril (Oct 31 2019 at 16:26, on Zulip):

@simulacrum I think that if a new crater run doesn't turn up anything new for some gates then those should be re-reverted again basically

centril (Oct 31 2019 at 16:27, on Zulip):

alternatives like those pertaining to tt are non-trivial architectural changes, possibly in need to lang RFCs

centril (Oct 31 2019 at 16:28, on Zulip):

the PR shouldn't be reverted fully; https://github.com/rust-lang/rust/pull/65742/commits/2e64bb2d37d5f15113e0a7199cd684504c6b8de7 needs to stay

simulacrum (Oct 31 2019 at 16:28, on Zulip):

I disagree, I think, though I'd want to think more... but I also don't have any more time today to discuss this.

(It, to me, seems odd to suggest that more specific, and as such less likely to cause breakage, forms of this change would need lang RFCs, while file-level tracking is fine without an RFC).

centril (Oct 31 2019 at 16:30, on Zulip):

the tt change is an architectural one; and would have big implications for future language design; specific changes to one other the other gates are not; and pre-expansion gating those is a conservative default

eddyb (Oct 31 2019 at 16:59, on Zulip):

FWIW I'm testing my take on the partial revert right now, it went pretty smooth so far

centril (Oct 31 2019 at 17:03, on Zulip):

@eddyb please cc me on the PR

eddyb (Oct 31 2019 at 17:35, on Zulip):

ahh, I see why this was so easy: we already had early feature-gating, @centril only moved some other feature-gates to use the same system

eddyb (Oct 31 2019 at 17:43, on Zulip):

@nikomatsakis @simulacrum opened #66004

Josh Triplett (Nov 06 2019 at 07:50, on Zulip):

So, I just caught up on this entire thread, and I'm confused.

Josh Triplett (Nov 06 2019 at 07:50, on Zulip):

First, I'm not sure what issue this is fixing. Any good source I could read for background on why we'd want to consider this?

Josh Triplett (Nov 06 2019 at 07:51, on Zulip):

More importantly, I'm concerned about any change that would require moving cfg outwards to outside an entire module.

Josh Triplett (Nov 06 2019 at 07:51, on Zulip):

In particular, consider the possibility that we may want to make mod unnecessary in the future.

Josh Triplett (Nov 06 2019 at 07:53, on Zulip):

One issue there: it would be helpful to move cfg inward, inside a module, so that you don't have to add an explicit mod statement just to attach a cfg to it.

Josh Triplett (Nov 06 2019 at 07:54, on Zulip):

But if we can't parse a file at all if it potentially uses new syntax, then we can't put #![cfg] inside the file.

Josh Triplett (Nov 06 2019 at 07:54, on Zulip):

I would like to better understand the rationale here, and evaluate it as a trade-off.

Josh Triplett (Nov 06 2019 at 07:55, on Zulip):

I'll also echo the feeling expressed by several here that this doesn't seem to have gotten discussed very visibly.

Josh Triplett (Nov 06 2019 at 07:56, on Zulip):

I agree with the proposal to revert and proceed more cautiously.

Last update: Nov 22 2019 at 05:15UTC