Add catch {} to AST #39921

cramertj · Feb 17, 2017

Part of #39849. Builds on #39864.

rust-highfive · Feb 17, 2017

r? @eddyb

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

cramertj · Feb 17, 2017

r? @nikomatsakis

cramertj · Feb 17, 2017

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

nikomatsakis · Feb 22, 2017

nikomatsakis reviewed Feb 22, 2017

View changes

nikomatsakis · Feb 22, 2017

src/test/compile-fail/catch-struct-name.rs

+#![allow(dead_code)]
+#![feature(catch_expr)]
+
+struct catch {} //~ ERROR expected identifier, found keyword `catch`


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 · Feb 22, 2017

nikomatsakis reviewed Feb 22, 2017

View changes

nikomatsakis · Feb 22, 2017

src/libsyntax/parse/parser.rs

@@ -468,6 +468,12 @@ impl<'a> Parser<'a> {
        }
    }

+    pub fn error_if_typename_is_catch(&mut self, ident: ast::Ident) {
+        if ident.name == keywords::Catch.name() {
+            self.span_err(self.span, "expected identifier, found keyword `catch`");


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

cramertj · Feb 22, 2017

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 · Feb 23, 2017

Trying a crater run for commits fc6f092 to e2f0332.

cramertj · Feb 23, 2017

cramertj reviewed Feb 23, 2017

View changes

cramertj · Feb 23, 2017

src/libsyntax/parse/parser.rs

@@ -3782,8 +3782,13 @@ impl<'a> Parser<'a> {
    }

    fn is_catch_expr(&mut self) -> bool {
-        self.token.is_keyword(keywords::Catch) &&
-        self.look_ahead(1, |t| *t == token::OpenDelim(token::Brace))
+        if self.restrictions.contains(Restrictions::RESTRICTION_NO_STRUCT_LITERAL) {


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

bors · Feb 25, 2017

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

cramertj · Feb 27, 2017

@nikomatsakis any word from crater?

KalitaAlexey · Feb 27, 2017

KalitaAlexey suggested changes Feb 27, 2017

View changes

KalitaAlexey · Feb 27, 2017

src/test/compile-fail/catch-struct-name.rs

+#![allow(dead_code)]
+#![feature(catch_expr)]
+
+struct catch {} //~ ERROR cannot use `catch` as the name of a type


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 · Feb 27, 2017

src/test/run-pass/catch-expr.rs

+    match catch {
+        _ => {}
+    };
+}


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 · Feb 27, 2017

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

bors · Feb 28, 2017

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

nikomatsakis · Mar 1, 2017

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

nikomatsakis · Mar 1, 2017

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 · Mar 2, 2017

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 · Mar 2, 2017

@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 · Mar 2, 2017

@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 · Mar 2, 2017

@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 · Mar 2, 2017

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 · Mar 2, 2017

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

cramertj · Mar 12, 2017

@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 · Mar 12, 2017

⌛️ Testing commit b1aa993 with merge 1f982c7...

bors · Mar 12, 2017

💔 Test failed - status-travis

eddyb · Mar 12, 2017

@bors retry

network issues #40474

bors · Mar 12, 2017

⌛️ Testing commit b1aa993 with merge 1174165...

bors · Mar 13, 2017

💔 Test failed - status-travis

cramertj · Mar 13, 2017

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 · Mar 13, 2017

I'll wait for @alexcrichton this time

alexcrichton · Mar 13, 2017

@bors: retry

spurious sccache failures #40240

bors · Mar 13, 2017

⌛️ Testing commit b1aa993 with merge 6be9825...

bors · Mar 13, 2017

💔 Test failed - status-travis

cramertj · Mar 13, 2017

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 · Mar 14, 2017

@bors: retry

spurious sccache failure #40240

bors · Mar 14, 2017

⌛️ Testing commit b1aa993 with merge 6f10e2f...

bors · Mar 14, 2017

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

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 cramertj:add-catch-to-ast branch from 7e5431f to cb565df Feb 18, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from cb565df to 11a322f Feb 22, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from 11a322f to e2f0332 Feb 23, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from d592d34 to 87917bd Feb 25, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from 87917bd to 6e94329 Feb 28, 2017

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

Auto merge of #39921 - cramertj:add-catch-to-ast, r=nikomatsakis
Add catch {} to AST Part of #39849. Builds on #39864.

Loading status checks…

1f982c7

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

Auto merge of #39921 - cramertj:add-catch-to-ast, r=nikomatsakis
Add catch {} to AST Part of #39849. Builds on #39864.

Loading status checks…

1174165

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

Auto merge of #39921 - cramertj:add-catch-to-ast, r=nikomatsakis
Add catch {} to AST Part of #39849. Builds on #39864.

Loading status checks…

6be9825

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

Auto merge of #39921 - cramertj:add-catch-to-ast, r=nikomatsakis
Add catch {} to AST Part of #39849. Builds on #39864.

Loading status checks…

6f10e2f

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Mar 14, 2017

Closed

Set CompilerDesugaring expn_info for all desugared expressions #39766

Merged

#[used] attribute #39987

Merged

Add feature gate for rvalue-static-promotion #40441

cramertj deleted the cramertj:add-catch-to-ast branch Mar 14, 2017

cramertj referenced this pull request Mar 15, 2017
Open
Mark `do` as a keyword and `do catch` as a control keyword. #102

Arnavion referenced this pull request Mar 16, 2017
Closed
Can't install clippy with cargo: pattern `Catch(_)` not covered #1622

alexcrichton referenced this pull request Mar 21, 2017
Merged
Rollup of 10 pull requests #40693

dtolnay referenced this pull request Apr 19, 2017
Closed
Catch expressions added to AST #109

withoutboats referenced this pull request May 31, 2017
Open
Tracking issue for `?` operator and `try` blocks (RFC 243, `question_mark` & `try_blocks` features) #31436
9 of 10 tasks complete

rust-lang/rust

Join GitHub today

Add catch {} to AST #39921

Conversation

cramertj commented Feb 17, 2017

rust-highfive assigned eddyb Feb 17, 2017

This comment has been minimized.

rust-highfive commented Feb 17, 2017

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

This comment has been minimized.

cramertj commented Feb 17, 2017

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

This comment has been minimized.

cramertj commented Feb 17, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from 7e5431f to cb565df Feb 18, 2017

nikomatsakis reviewed Feb 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

nikomatsakis reviewed Feb 22, 2017 View changes

This comment has been minimized.

cramertj force-pushed the cramertj:add-catch-to-ast branch from cb565df to 11a322f Feb 22, 2017

This comment has been minimized.

cramertj commented Feb 22, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from 11a322f to e2f0332 Feb 23, 2017

This comment has been minimized.

nikomatsakis commented Feb 23, 2017

cramertj reviewed Feb 23, 2017 View changes

This comment has been minimized.

This comment has been minimized.

bors commented Feb 25, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from d592d34 to 87917bd Feb 25, 2017

This comment has been minimized.

cramertj commented Feb 27, 2017

KalitaAlexey suggested changes Feb 27, 2017 View changes

This comment has been minimized.

This comment has been minimized.

cramertj Feb 28, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Feb 27, 2017

This comment has been minimized.

bors commented Feb 28, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from 87917bd to 6e94329 Feb 28, 2017

This comment has been minimized.

nikomatsakis commented Mar 1, 2017

This comment has been minimized.

nikomatsakis commented Mar 1, 2017

This comment has been minimized.

nikomatsakis commented Mar 2, 2017 • edited

This comment has been minimized.

Marwes commented Mar 2, 2017

This comment has been minimized.

nikomatsakis commented Mar 2, 2017

This comment has been minimized.

cramertj commented Mar 2, 2017 • edited

This comment has been minimized.

nrc commented Mar 2, 2017

This comment has been minimized.

bors commented Mar 2, 2017

This comment has been minimized.

cramertj commented Mar 12, 2017

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

This comment has been minimized.

bors commented Mar 12, 2017

This comment has been minimized.

bors commented Mar 12, 2017

This comment has been minimized.

eddyb commented Mar 12, 2017 • edited by alexcrichton

This comment has been minimized.

bors commented Mar 12, 2017

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

This comment has been minimized.

bors commented Mar 13, 2017

This comment has been minimized.

cramertj commented Mar 13, 2017

This comment has been minimized.

eddyb commented Mar 13, 2017

This comment has been minimized.

cramertj force-pushed the cramertj:add-catch-to-ast branch from `7e5431f` to `cb565df` Feb 18, 2017

nikomatsakis reviewed Feb 22, 2017

View changes

nikomatsakis reviewed Feb 22, 2017

View changes

cramertj force-pushed the cramertj:add-catch-to-ast branch from `cb565df` to `11a322f` Feb 22, 2017

cramertj force-pushed the cramertj:add-catch-to-ast branch from `11a322f` to `e2f0332` Feb 23, 2017

cramertj reviewed Feb 23, 2017

View changes

cramertj force-pushed the cramertj:add-catch-to-ast branch from `d592d34` to `87917bd` Feb 25, 2017

KalitaAlexey suggested changes Feb 27, 2017

View changes

cramertj Feb 28, 2017 •

edited

cramertj force-pushed the cramertj:add-catch-to-ast branch from `87917bd` to `6e94329` Feb 28, 2017

nikomatsakis commented Mar 2, 2017 •

edited

cramertj commented Mar 2, 2017 •

edited

eddyb commented Mar 12, 2017 •

edited by alexcrichton

cramertj commented Mar 13, 2017 •

edited

bors merged commit `b1aa993` into rust-lang:master Mar 14, 2017
2 checks passed