libsyntax: note that `let a = (let b = something)` is invalid #30765

dsprenkels · 2016-01-07T16:50:47Z

This PR is in response to #30440.

rust-highfive · 2016-01-07T16:51:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

dsprenkels · 2016-01-07T16:54:09Z

r? @Manishearth

wafflespeanut · 2016-01-07T17:17:08Z

Now, we should update the existing test src/test/parse-fail/keyword.rs with a main function having the wrong expression and check whether this works.

Manishearth · 2016-01-07T17:52:15Z

Alternatively, add a new parse-fail test.

dsprenkels · 2016-01-07T21:09:13Z

Well, at the moment I still have the issue with the following code:

fn main() {
  let let = 3;
  println!(let);
}

which of course fails:

main.rs:2:7: 2:10 error: expected identifier, found keyword `let`
main.rs:2   let let = 3;
                ^~~
main.rs:2:7: 2:10 note: `let` is not an expression, so it cannot be used in this way
main.rs:2   let let = 3;
                ^~~
error: aborting due to previous error

However, the note that let is not an expression is only confusing here, ~~but I don't know how to know that we are dealing with this case~~ so I changed the location of the code to the expression parsing function.

in parse_bottom_expr (parser.rs)

Manishearth · 2016-01-08T08:44:29Z

run-fail-fulldeps/qquote.rs fails. Might want to run TESTNAME=qquote make check-stage1-rfail-full to run it individually and try

wafflespeanut · 2016-01-08T09:18:27Z

@dsprenkels I'm not sure why you want to address that case, because the compiler already points it out quite correctly (that it expects an identifier and not a keyword). I think your previous version was good (since it addresses the actual issue)

For building & testing, run make -j4 check-stage1-pfail TESTNAME=keyword.rs, which checks the keyword.rs file in parse-fail directory. Also, have a look at the neighboring tests for the pattern expected by make for these tests. In our case, the erroneous line requires //~ NOTE let is not an expression ...

dsprenkels · 2016-01-08T09:31:36Z

No, the previous fix does complain about let let = 3 (and adds a note). I indeed do not want to address this issue, only let a = (let b = 3) should trigger this custom note. That's why I moved the code to parse_bottom_expr.

~~By the way, it could be that I don't understand what you mean.~~ 😃

wafflespeanut · 2016-01-08T09:58:50Z

Oh, fine. Then, carry on :)

… let error, by moving unexpected let check into the proper if-else clause

dsprenkels · 2016-01-17T13:49:06Z

Currently, the qquote test fails, because of this change:

no-location error: `let` is not an expression, so it cannot be used in this way
thread '<main>' panicked at 'position 0 does not resolve to a source location', src/libsyntax/codemap.rs:1049
error: internal compiler error: Error constructed but not emitted
thread '<main>' panicked at 'explicit panic', src/libsyntax/errors/mod.rs:261

It is, however, unclear to me how to resolve this, as I do not know this plugin works.

Manishearth · 2016-01-17T13:51:26Z

cc @eddyb @nrc I'm not familiar with how qquote works.

nrc · 2016-01-17T20:25:14Z

src/libsyntax/parse/parser.rs

+                } else if self.token.is_keyword(keywords::Let) {
+                    // Catch this syntax error here, instead of in `check_strict_keywords`, so
+                    // that we can explicitly mention that let is not to be used as an expression
+                    let msg = "`let` is not an expression, so it cannot be used in this way";


This error message needs some work: why single let out here? Rather than other keywords, it seems oddly specific. You probably mean that "a let statement is not an expression", not that let it self is not. It is not clear what "this way" means.

Maybe better, something like: expected expression, found statement; variable declaration usingletis a statement. Perhaps the bit after the semicolon should be a note.

Note that neither then before or after message fit the let let x = ... case, where we expect an identifier, not a keyword, so the original error is better, and the new one (either as well as or instead of) is wrong.

nrc · 2016-01-17T20:31:08Z

error: internal compiler error: Error constructed but not emitted

This error is probably only signalled because of the first panic. Would be good to get a stack trace for the first panic. Try running the test with RUST_BACKTRACE=1, then post the stack trace here.

The first error is probably because you're making an error which includes the span, but the span for quasi-quoted code is not a valid one. Not sure how we handle this usually.

dsprenkels · 2016-01-17T21:29:08Z

Full traceback:

no-location error: `let` is not an expression, so it cannot be used in this way
thread '<main>' panicked at 'position 0 does not resolve to a source location', src/libsyntax/codemap.rs:1049
error: internal compiler error: Error constructed but not emitted
thread '<main>' panicked at 'explicit panic', src/libsyntax/errors/mod.rs:261
stack backtrace:
   1:     0x7f2cdd6439d0 - sys::backtrace::tracing::imp::write::h01407790575153d2Afu
   2:     0x7f2cdd645d95 - panicking::default_handler::_<closure>::closure.42892
   3:     0x7f2cdd64583f - panicking::default_handler::h55b20e8b59558085Eqy
   4:     0x7f2cdd639c96 - sys_common::unwind::begin_unwind_inner::h3c652ec223587f3d97s
   5:     0x7f2cdd41b67f - sys_common::unwind::begin_unwind::h16145324357146396763
   6:     0x7f2cdd41d953 - errors..DiagnosticBuilder::drop.56135::hbe7c46c12efc554c
   7:     0x7f2cdd60eed3 - ext::quote::parse_expr_panic::h40b20d48d67152e8jWb
   8:     0x7f2cdd3fc9d7 - main::h01aa5d7192b94c41laa
   9:     0x7f2cdd6454c4 - sys_common::unwind::try::try_fn::h1519701836809394149
  10:     0x7f2cdd642e78 - __rust_try
  11:     0x7f2cdd645166 - rt::lang_start::h9cf98e5c72fa4d24Siy
  12:     0x7f2cdd41af19 - main
  13:     0x7f2cdc805b44 - __libc_start_main
  14:     0x7f2cdd3fb9e0 - <unknown>
  15:                0x0 - <unknown>
thread panicked while panicking. aborting.Illegal instruction

Furthermore: I agree that the error message is not very informative at the moment. The original plan was (still is?) to add a note to the error thrown in check_strict_keywords, but only if the keyword is let, and if it is used at the beginning of an expression, but I really do not know how to check in check_strict_keywords if we are parsing at the beginning of an expression.

nrc · 2016-01-17T23:07:50Z

Hmm, this is puzzling. It is clear what is going on - when quasi-quoting we are getting an error causing us to panic. However, it is not clear why the error is not emited - panictry! should do that. Furthermore, its not clear why your change should change anything, iiuc, only the message changes (unless it was previously not a fatal error? But I think all parsing errors are fatal).

dsprenkels · 2016-01-18T08:50:54Z

Furthermore, its not clear why your change should change anything, iiuc, only the message changes (unless it was previously not a fatal error? But I think all parsing errors are fatal).

The new error is indeed fatal, whereas the error that would originally occur was not.

Manishearth · 2016-01-25T22:22:29Z

As I see it, the issue here is this: The parser maintains a self.span. When you use self.fatal(..), this span is used. However, in the case of quasiquoting, this span is in fact invalid (it's obtained from read_tok(), not sure how valid that is), which leads to the "position 0 does not resolve to a source location" bug.

I noticed that emit_ is being called in the EmitterWriter, however that's only supposed to be called when you have a proper span.

The relevant backtrace is:

thread '<main>' panicked at 'position 0 does not resolve to a source location', src/libsyntax/codemap.rs:1064

Breakpoint 1, 0x000055555577abf0 in rust_panic ()
(gdb) bt
#0  0x000055555577abf0 in rust_panic ()
#1  0x0000555555775f83 in sys_common::unwind::begin_unwind_inner::h5a8431dee636759bfgt ()
#2  0x0000555555776369 in sys_common::unwind::begin_unwind_fmt::hfa6b9a8c65cd0705lft ()
#3  0x00005555555bc85e in codemap::CodeMap::lookup_filemap_idx::h3b3fd7c2fa2bb1c3TFD ()
#4  0x00005555555bc643 in codemap::CodeMap::bytepos_to_file_charpos::h168ad2fe9d3430b6oCD ()
#5  0x000055555559679d in codemap::CodeMap::lookup_char_pos::h4159a17f5570f91cgeD ()
#6  0x0000555555595224 in codemap::CodeMap::span_to_lines::hb0a634d700a83c418uD ()
#7  0x000055555558fab2 in errors::emitter::EmitterWriter::emit_::h30e316131ba94c17B5b ()
#8  0x000055555558dfe6 in errors::emitter::EmitterWriter.Emitter::emit::hd5a2f310c1cd3936A1b ()
#9  0x000055555558e27b in errors::emitter::Emitter::emit_struct::h10020655746231161034 ()
#10 0x00005555557462a3 in ext::quote::parse_expr_panic::h17ebcf9dc12ef60fltb ()
#11 0x000055555556748b in foo::main () at foo.rs:29
#12 0x00005555557812e5 in sys_common::unwind::try::try_fn::h3804301371837416440 ()
#13 0x000055555577ebb9 in __rust_try ()
#14 0x0000555555780f1e in rt::lang_start::h620f739da2fd976doxy ()
#15 0x0000555555584fca in main ()

@nrc any idea what's going on here?

nrc · 2016-01-26T04:02:14Z

That sounds like a reasonable analysis. We have access to the error before it is emitted in quote::parse_expr_panic, so we could strip out the span from the error there. Alternatively, we could check that the span is valid in EmitterWriter::emit and don't try and use it if not. The second is more robust and probably better.

Manishearth · 2016-01-26T08:25:15Z

I fixed the error in https://github.com/rust-lang/rust/compare/master...Manishearth:pr-30765?expand=1, and additionally used the improved error message. Should I make a new PR?

Otherwise cherry-picking the last two commits should work.

Manishearth · 2016-01-26T09:01:24Z

Superseded by #31211.

dsprenkels · 2016-01-26T09:12:01Z

@Manishearth Thank you! Shall I close this PR in favor of #31211?

Manishearth · 2016-01-26T09:29:04Z

Sure. I opened a new PR in case you didn't want to cherry pick. It's also acceptable to cherry pick commits here and we'll close that PR 😄

dsprenkels · 2016-01-26T09:31:17Z

Closed in favor of #31211.

Manishearth · 2016-01-26T15:35:25Z

This merged (#31211).

Thanks for all the effort, @dsprenkels, and apologies for the holdups! 😃

rust-highfive assigned alexcrichton Jan 7, 2016

rust-highfive assigned Manishearth and unassigned alexcrichton Jan 7, 2016

dsprenkels added 2 commits January 7, 2016 23:28

libsyntax: note that let a = (let b = something) is invalid

7229ca9

in parse_bottom_expr (parser.rs)

libsyntax: move check for keyword Let to a more logical spot

a74585b

dsprenkels added 2 commits January 14, 2016 16:04

Update qquote.rs test case and make unexpected let error fatal

8c74957

do not additionally note about unexpected identifier after unexpected…

740f2f3

… let error, by moving unexpected let check into the proper if-else clause

nrc reviewed Jan 17, 2016
View reviewed changes

Manishearth mentioned this pull request Jan 26, 2016

(take 2) libsyntax: note that let a = (let b = something) is invalid #31211

Merged

dsprenkels closed this Jan 26, 2016

dsprenkels deleted the unexpected-let-keyword branch January 26, 2016 09:31

dsprenkels restored the unexpected-let-keyword branch January 26, 2016 09:34

dsprenkels deleted the unexpected-let-keyword branch January 26, 2016 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libsyntax: note that `let a = (let b = something)` is invalid #30765

libsyntax: note that `let a = (let b = something)` is invalid #30765

dsprenkels commented Jan 7, 2016

rust-highfive commented Jan 7, 2016

dsprenkels commented Jan 7, 2016

wafflespeanut commented Jan 7, 2016

Manishearth commented Jan 7, 2016

dsprenkels commented Jan 7, 2016

Manishearth commented Jan 8, 2016

wafflespeanut commented Jan 8, 2016

dsprenkels commented Jan 8, 2016

wafflespeanut commented Jan 8, 2016

dsprenkels commented Jan 17, 2016

Manishearth commented Jan 17, 2016

nrc Jan 17, 2016

nrc commented Jan 17, 2016

dsprenkels commented Jan 17, 2016

nrc commented Jan 17, 2016

dsprenkels commented Jan 18, 2016

Manishearth commented Jan 25, 2016

nrc commented Jan 26, 2016

Manishearth commented Jan 26, 2016

Manishearth commented Jan 26, 2016

dsprenkels commented Jan 26, 2016

Manishearth commented Jan 26, 2016

dsprenkels commented Jan 26, 2016

Manishearth commented Jan 26, 2016

libsyntax: note that let a = (let b = something) is invalid #30765

libsyntax: note that let a = (let b = something) is invalid #30765

Conversation

dsprenkels commented Jan 7, 2016

rust-highfive commented Jan 7, 2016

dsprenkels commented Jan 7, 2016

wafflespeanut commented Jan 7, 2016

Manishearth commented Jan 7, 2016

dsprenkels commented Jan 7, 2016

Manishearth commented Jan 8, 2016

wafflespeanut commented Jan 8, 2016

dsprenkels commented Jan 8, 2016

wafflespeanut commented Jan 8, 2016

dsprenkels commented Jan 17, 2016

Manishearth commented Jan 17, 2016

nrc Jan 17, 2016

Choose a reason for hiding this comment

nrc commented Jan 17, 2016

dsprenkels commented Jan 17, 2016

nrc commented Jan 17, 2016

dsprenkels commented Jan 18, 2016

Manishearth commented Jan 25, 2016

nrc commented Jan 26, 2016

Manishearth commented Jan 26, 2016

Manishearth commented Jan 26, 2016

dsprenkels commented Jan 26, 2016

Manishearth commented Jan 26, 2016

dsprenkels commented Jan 26, 2016

Manishearth commented Jan 26, 2016

libsyntax: note that `let a = (let b = something)` is invalid #30765

libsyntax: note that `let a = (let b = something)` is invalid #30765