Add catch {} to AST by cramertj · Pull Request #39921 · rust-lang/rust

cramertj · 2017-02-17T23:14:51Z

Part of #39849. Builds on #39864.

rust-highfive · 2017-02-17T23:14:58Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

cramertj · 2017-02-17T23:15:11Z

r? @nikomatsakis

cramertj · 2017-02-17T23:24:50Z

Note that only the last commit of this is new-- the rest will be rebased away once #39864 is merged in.

nikomatsakis · 2017-02-22T13:52:07Z

Can you add a test for local variables? Just want to make sure that this works:

fn main() { let catch = 0; }

It'd also might be good to see if we can get a better error here (i.e., "catch cannot be used as a struct name").

Also, we need a test for enum variants:

enum Foo { catch { ... } }

We should also test for unit- and tuple-like like structs/variants (struct catch, struct catch(u32), etc).

nikomatsakis · 2017-02-22T13:52:41Z

I think I prefer "cannot use catch as the name of a type"

cramertj · 2017-02-22T20:58:58Z

Github, why do you insist on ordering my commits in chronological order rather than log order? For those reading who are confused: the new commits in this PR are Add catch expr to AST and Fix review nits.

nikomatsakis · 2017-02-23T20:44:11Z

Trying a crater run for commits fc6f092 to e2f0332077ec6ddcf4239ec68dc6a8324b485600.

cramertj · 2017-02-23T22:10:16Z

Not that it matters, but this would be cleaner as !self.restrictions.contains(...) && self.token.is_keyword(keywords::Catch) && ...

bors · 2017-02-25T15:08:02Z

☔ The latest upstream changes (presumably #40091) made this pull request unmergeable. Please resolve the merge conflicts.

cramertj · 2017-02-27T06:45:10Z

@nikomatsakis any word from crater?

KalitaAlexey · 2017-02-27T08:00:00Z

Why did you separate struct catch and struct catch {}?
Can't you place them in a single file?

Once the first one has been hit, no more errors are generated.

KalitaAlexey · 2017-02-27T08:01:35Z

Could you add a test for:

let catch_result = catch { let catch = false; catch };

Will that work?

Yes, it would work, but I don't see anything particularly special about that case.

I don't know.

nikomatsakis · 2017-02-27T16:57:55Z

@cramertj no, things keep going awry somehow. I'm going to see if I can get @brson to investigate.

bors · 2017-02-28T10:05:05Z

☔ The latest upstream changes (presumably #40008) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-03-01T14:55:50Z

I'm trying yet another crater run. I keep getting timeouts. Perhaps this latest round was due to S3.

nikomatsakis · 2017-03-01T23:01:56Z

And...I'm an idiot. I've been fat-fingering the URL to your repo @cramertj, sorry. Hopefully my new crater runs will work better. =)

nikomatsakis · 2017-03-02T09:41:25Z

OK, here are the results:

https://gist.github.com/nikomatsakis/d26c0ac1a6ba2ebfd6164010a679b57a

The TL;DR is that there is one package found to be affected, which is the gluon-lang package (cc @Marwes). I am debating the best way to go forward here, given that crater represents an incomplete glimpse into existing Rust code. (cc @rust-lang/lang)

(Context for @Marwes and @rust-lang/lang : the ? RFC wants to introduce a catch { ... } syntax, which requires us to prohibit structs named catch; gluon-parser seems to have such a struct.)

Marwes · 2017-03-02T15:20:25Z

@nikomatsakis It is actually this macro https://github.com/gluon-lang/gluon/blob/master/vm/src/api.rs#L1379-L1392 called from https://github.com/gluon-lang/gluon/blob/3a12f66d741ba843e5e85fa8f8d18ef7438284f7/src/io.rs#L222 that is the error. The struct isn't used directly so the struct's name isn't important as it is only a "field name" for a record type ({ catch : a -> b }).

It wouldn't be impossible for me to make the macro avoid generating structs as it does now but it isn't entirely trivial either.

nikomatsakis · 2017-03-02T18:42:28Z

@cramertj so I think we need to have a bigger discussion about keywords. I'm feeling a bit less comfortable with the "just change it" -- though that may be the eventual decision. I'm wondering if, to keep this implementation going forward, we should consider something hacky and temporary, such as one of the following:

using __catch__ { ... } for now, or however many underscores we need to feel confident,
requiring a -Zenable-catch catch flag in the compiler to turn it on,
or doing something similar with the feature-gate, which seems harder.

cramertj · 2017-03-02T19:56:43Z

@nikomatsakis If we're going to make something up to avoid breakage, why not just prefix it with an unused keyword? (e.g do catch { }). That way we avoid any chance of breakage at all. (Edit: to clarify, I'm only proposing this as a stop-gap measure, not a final solution)

As for the eventual solution, I'm leaning towards "just change it." This feels like pretty minor breakage, and the motivations are clear.

WRT the bigger conversation about keywords, I think keyword additions in RFCs must be viewed as breaking changes unless they are accompanied by a disambiguation strategy. Conversations like the one we're having here should be part of the RFC process.

nrc · 2017-03-02T20:18:16Z

I think this should count as a 'minor breaking change' i.e., we'll do it with a warning cycle, but still break people without bumping the major version number. Because:

I think any solution using something more verbose than catch for the keyword would be very harmful for ergonomics.
We have pretty strong conventions that struct names start with a capital letter, so I think it would be very rare for anyone to hit this (the crater run seems to support this).
It is an easy fix (simple rename, which can be done automatically by the RLS :-), or easily with search + replace )
we could have an enable-catch flag temporarily (e.g., during the warning period) but I'm strongly against such flags in the long run - too much risk of 'dialects' of Rust emerging.

bors · 2017-03-02T22:20:22Z

☔ The latest upstream changes (presumably #40216) made this pull request unmergeable. Please resolve the merge conflicts.

cramertj · 2017-03-12T22:35:27Z

@eddyb Yeah, I know. haha. I'm just getting tired of bugging other ppl about it and thought I'd try yelling at bors myself.

bors · 2017-03-12T22:35:28Z

⌛ Testing commit b1aa993 with merge 1f982c7...

bors · 2017-03-12T23:10:07Z

💔 Test failed - status-travis

eddyb · 2017-03-12T23:15:18Z

@bors retry

network issues Tracking issue for spurious network failures on bots #40474

bors · 2017-03-12T23:22:13Z

⌛ Testing commit b1aa993 with merge 1174165...

bors · 2017-03-13T02:24:04Z

💔 Test failed - status-travis

cramertj · 2017-03-13T03:30:51Z

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

eddyb · 2017-03-13T08:18:43Z

I'll wait for @alexcrichton this time

alexcrichton · 2017-03-13T16:04:08Z

@bors: retry

spurious sccache failures Spurious failures in sccache #40240

bors · 2017-03-13T18:56:47Z

⌛ Testing commit b1aa993 with merge 6be9825...

bors · 2017-03-13T19:34:10Z

💔 Test failed - status-travis

cramertj · 2017-03-13T20:13:07Z

Travis says:

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/PDBFile.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32m�[1mLinking CXX executable ../../bin/llvm-mcmarkup�[0m

DEBUG:sccache::commands: Server sent UnhandledCompile

error: failed to execute compile

caused by: failed to spawn child

caused by: Broken pipe (os error 32)

make[3]: *** [bin/llvm-mcmarkup] Error 2

make[3]: *** Deleting file `bin/llvm-mcmarkup'

make[2]: *** [tools/llvm-mcmarkup/CMakeFiles/llvm-mcmarkup.dir/all] Error 2

make[2]: *** Waiting for unfinished jobs....

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/PDBFileBuilder.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/PublicsStream.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/RawError.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/RawSession.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/SymbolStream.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 22%] �[32mBuilding CXX object lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Raw/TpiStream.cpp.o�[0m

DEBUG:sccache::commands: Server sent CompileStarted

[ 23%] �[32m�[1mLinking CXX static library ../../libLLVMDebugInfoPDB.a�[0m

[ 23%] Built target LLVMDebugInfoPDB

make[1]: *** [all] Error 2

thread 'main' panicked at '

command did not execute successfully, got: exit code: 2



build script failed, must exit now', /Users/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.21/src/lib.rs:605

note: Run with `RUST_BACKTRACE=1` for a backtrace.

	finished in 159.685

Build completed unsuccessfully in 0:03:07

cc @alexcrichton

alexcrichton · 2017-03-14T02:55:09Z

@bors: retry

spurious sccache failure Spurious failures in sccache #40240

bors · 2017-03-14T10:40:14Z

⌛ Testing commit b1aa993 with merge 6f10e2f...

Add catch {} to AST Part of #39849. Builds on #39864.

bors · 2017-03-14T13:23:20Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6f10e2f to master...

@nikomatsakis

…sakis Implement `?` in catch expressions Builds on rust-lang#39921. Final part of rust-lang#39849. r? @nikomatsakis

rust-highfive assigned eddyb Feb 17, 2017

cramertj changed the title ~~Add Catch to AST~~ Add catch {} to AST Feb 17, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Feb 17, 2017

cramertj force-pushed the add-catch-to-ast branch from 7e5431f to cb565df Compare February 18, 2017 00:22

nikomatsakis reviewed Feb 22, 2017

View reviewed changes

cramertj force-pushed the add-catch-to-ast branch from cb565df to 11a322f Compare February 22, 2017 20:55

cramertj force-pushed the add-catch-to-ast branch from 11a322f to e2f0332 Compare February 23, 2017 04:58

cramertj commented Feb 23, 2017

View reviewed changes

cramertj force-pushed the add-catch-to-ast branch from d592d34 to 87917bd Compare February 25, 2017 21:29

KalitaAlexey suggested changes Feb 27, 2017

View reviewed changes

cramertj force-pushed the add-catch-to-ast branch from 87917bd to 6e94329 Compare February 28, 2017 19:27

bors added a commit that referenced this pull request Mar 14, 2017

Auto merge of #39921 - cramertj:add-catch-to-ast, r=nikomatsakis

6f10e2f

Add catch {} to AST Part of #39849. Builds on #39864.

bors merged commit b1aa993 into rust-lang:master Mar 14, 2017

This was referenced Mar 14, 2017

Set CompilerDesugaring expn_info for all desugared expressions #39766

Closed

#[used] attribute #39987

Merged

Add feature gate for rvalue-static-promotion #40441

Merged

cramertj deleted the add-catch-to-ast branch March 14, 2017 15:30

cramertj mentioned this pull request Mar 15, 2017

Mark do as a keyword and do catch as a control keyword. zargony/atom-language-rust#102

Open

Arnavion mentioned this pull request Mar 16, 2017

Can't install clippy with cargo: pattern Catch(_) not covered rust-lang/rust-clippy#1622

Closed

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 21, 2017

Rollup merge of rust-lang#40229 - cramertj:break-to-blocks, r=nikomat…

fcc2d25

…sakis Implement `?` in catch expressions Builds on rust-lang#39921. Final part of rust-lang#39849. r? @nikomatsakis

alexcrichton mentioned this pull request Mar 21, 2017

Rollup of 10 pull requests #40693

Merged

dtolnay mentioned this pull request Apr 19, 2017

Catch expressions added to AST dtolnay/syn#109

Closed

withoutboats mentioned this pull request May 31, 2017

Tracking issue for ? operator (RFC 243, question_mark) #31436

Closed

16 tasks

Uh oh!

Conversation

cramertj commented Feb 17, 2017

Uh oh!

rust-highfive commented Feb 17, 2017

Uh oh!

cramertj commented Feb 17, 2017

Uh oh!

cramertj commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramertj commented Feb 22, 2017

Uh oh!

nikomatsakis commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Feb 25, 2017

Uh oh!

cramertj commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramertj Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Feb 27, 2017

Uh oh!

bors commented Feb 28, 2017

Uh oh!

nikomatsakis commented Mar 1, 2017

Uh oh!

nikomatsakis commented Mar 1, 2017

Uh oh!

nikomatsakis commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Marwes commented Mar 2, 2017

Uh oh!

nikomatsakis commented Mar 2, 2017

Uh oh!

cramertj commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented Mar 2, 2017

Uh oh!

bors commented Mar 2, 2017

Uh oh!

cramertj commented Mar 12, 2017

Uh oh!

bors commented Mar 12, 2017

Uh oh!

bors commented Mar 12, 2017

Uh oh!

eddyb commented Mar 12, 2017 • edited by alexcrichton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Mar 12, 2017

Uh oh!

bors commented Mar 13, 2017

Uh oh!

cramertj commented Mar 13, 2017

Uh oh!

eddyb commented Mar 13, 2017

Uh oh!

alexcrichton commented Mar 13, 2017

Uh oh!

bors commented Mar 13, 2017

cramertj Feb 28, 2017 •

edited

Loading

nikomatsakis commented Mar 2, 2017 •

edited

Loading

cramertj commented Mar 2, 2017 •

edited

Loading

eddyb commented Mar 12, 2017 •

edited by alexcrichton

Loading

cramertj commented Mar 13, 2017 •

edited

Loading