Move E0379 check from typeck to ast validation #35480

KiChjang · 2016-08-08T00:26:20Z

Part of #35233.
Extension of #35338, #35364.
Fixes #35404.

KiChjang · 2016-08-08T00:28:21Z

src/libsyntax/ext/tt/macro_rules.rs

@@ -94,7 +94,7 @@ impl<'a> MacResult for ParserAnyMacro<'a> {
            let mut parser = self.parser.borrow_mut();
            match parser.token {
                token::Eof => break,
-                _ => ret.push(panictry!(parser.parse_impl_item()))
+                _ => ret.push(panictry!(parser.parse_impl_item(false)))


Someone should double-check this - make_items should cover the case for macro trait impl fns and trait fns, so when make_impl_items is called, we can be certain that this is going to be a plain impl (i.e. impl without traits), hence this here is false.

petrochenkov · 2016-08-08T08:27:28Z

@KiChjang
Checks performed in the parser can be overridden by syntax extensions (and occasionally macros), so we have a special pass instead that runs after expansion: https://github.com/rust-lang/rust/blob/master/src/librustc_passes/ast_validation.rs
This check is a perfect candidate for moving there.

KiChjang · 2016-08-10T10:08:35Z

Moved the error from the parser to the ast validator.

KiChjang · 2016-08-10T10:09:08Z

r? @jonathandturner

nikomatsakis · 2016-08-10T17:58:28Z

src/librustc_typeck/check/mod.rs

@@ -819,14 +819,10 @@ pub fn check_item_body<'a,'tcx>(ccx: &CrateCtxt<'a,'tcx>, it: &'tcx hir::Item) {
                    check_const(ccx, trait_item.span, &expr, trait_item.id)
                }
                hir::MethodTraitItem(ref sig, Some(ref body)) => {


can you add an assertion here that the fn is not const? (and maybe a comment pointing out that this is checked in the AST validator?)

But that will cause ICEs in erroneous code - the error in the validation pass is not fatal and compilation continues until type checking is done.

Yes, this would cause an ICE with const unsafe fn.

nikomatsakis · 2016-08-10T18:01:35Z

r? @nikomatsakis

nikomatsakis · 2016-08-10T19:43:38Z

@KiChjang

Could you show me where a span is created for an AST/HIR node? (probably not doing in this PR, but is good to know nonetheless)

Yes, that is generally in the parser (and, in that context, it's ok to do subtraction of this kind, though always a bit sketchy -- but in any case there we know that macro expansion hasn't happened yet, so the spans of different files and so forth are not mixed up). If you look in the parser, you'll see that it tracks lo (start points) and so forth and constructs spans and sticks them into various AST/HIR nodes.

If we wanted to track the span of just this keyword, then, I would probably change the AST to have Spanned<Constness> instead of Constness -- the type-checker will then guide you to the place that you have to add the span in the parser. (You'll also have to make misc changes elsewhere as a result, typically changing constness to constness.node (to skip past the span).)

nikomatsakis · 2016-08-10T20:17:49Z

OK, sorry, forget the assertions, you're right :)

nikomatsakis · 2016-08-10T20:18:18Z

In that case, can you just add some comments?

nikomatsakis · 2016-08-10T20:18:36Z

Meh, I guess that's not necessary.

nikomatsakis · 2016-08-10T20:34:15Z

(To be clear, I'd still like to see the "span munging" changed.)

KiChjang · 2016-08-10T21:30:35Z

Just a heads up, I've changed my mind. I'll be incorporating the changes to make Spanned<Constness> possible.

KiChjang · 2016-08-10T23:28:31Z

Okay, I've made it so that all Constness fields in the ast/hir are all Spanned<Constness> instead.

nikomatsakis · 2016-08-11T16:21:02Z

@KiChjang this all looks good; one thought is that if you convert the test into a ui test, we would be able to see (in the test) that the span is correct (right now the test harness is only checking line-numbers). There is some advice for writing ui tests (and generated the reference files) in src/test/ui/README.md.

What do you think?

petrochenkov · 2016-08-11T16:47:01Z

Adding a span to constness seems pretty random. Why does it have a span while unsafety doesn't, for example. Many more important AST fragments, like match arms, don't have spans. What is the general policy here?
Spans are heavy enough (3 * u32), @KiChjang have you checked if sizes of Item/ImplItem/TraitItem changed? Is an improvement to one error message important enough to pay the price if they did?
(Of course, It would be ideal to have spans for all tokens, but that would blow up the AST/HIR size unless some smart scheme is used, which is not the case now.)
Also, this is a plugin-breaking change now.

sophiajt · 2016-08-11T17:13:02Z

@petrochenkov - there can be multiple PRs that address different issues, so a later PR could add a span to unsafe, for example.

cc @nikomatsakis - since we were just chatting about AST nodes getting spans.

There may be a cost, which hopefully we can detect and address it if any exists, but the ergonomics of being able to have the spans and give better error messages is helpful.

KiChjang · 2016-08-11T17:38:27Z

@nikomatsakis I'm fine with adding them to this PR, but I think we'll need to make it so that everyone working on #35233 should also write UI tests, if they have tightened the spans for some errors.

nikomatsakis · 2016-08-11T19:03:12Z

@petrochenkov we are already somewhat inconsistent about where we include spans -- e.g., we sometimes put spans on identifiers, sometimes not. It seems... ok to me.

nikomatsakis · 2016-08-17T20:52:37Z

src/libsyntax/parse/parser.rs

@@ -5775,7 +5783,9 @@ impl<'a> Parser<'a> {
            // FUNCTION ITEM
            self.bump();
            let (ident, item_, extra_attrs) =
-                self.parse_item_fn(Unsafety::Normal, Constness::NotConst, Abi::Rust)?;
+                self.parse_item_fn(Unsafety::Normal,
+                                   dummy_spanned(Constness::NotConst),


KiChjang · 2016-08-17T21:11:18Z

Dummy spans removed from parser.rs.

nikomatsakis · 2016-08-18T19:07:28Z

@bors r+

bors · 2016-08-18T19:07:29Z

📌 Commit 9fae2f3 has been approved by nikomatsakis

KiChjang · 2016-08-18T19:34:36Z

Rebased.

sophiajt · 2016-08-19T16:22:27Z

Huh, looks like a test in the src/libsyntax/parse/mod.rs needs updating?

test parse::tests::parse_fundecl ... FAILED

KiChjang · 2016-08-19T22:29:05Z

Fixed the unit test in the latest force-push.

sophiajt · 2016-08-22T15:53:26Z

@bors r+ rollup

bors · 2016-08-22T15:53:27Z

📌 Commit b5b667d has been approved by jonathandturner

sophiajt · 2016-08-22T15:54:18Z

@bors r=nikomatsakis

bors · 2016-08-22T15:54:20Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: cstring: avoid excessive growth just to 0-terminate #35871

bors · 2016-08-22T15:54:21Z

📌 Commit b5b667d has been approved by nikomatsakis

jseyfried · 2016-08-22T19:08:43Z

@bors r-
This is a syntax-[breaking-change] and will land in the next batch (c.f. #31645 (comment)).

Move E0379 check from typeck to ast validation Part of rust-lang#35233. Extension of rust-lang#35338, rust-lang#35364. Fixes rust-lang#35404.

@Manishearth

Batch up libsyntax breaking changes Batch of the following syntax-[breaking-change] changes: - #35591: Add a field `span: Span` to `ast::Generics`. - #35618: Remove variant `Mod` of `ast::PathListItemKind` and refactor the remaining variant `ast::PathListKind::Ident` to a struct `ast::PathListKind_`. - #35480: Change uses of `Constness` in the AST to `Spanned<Constness>`. - c.f. `MethodSig`, `ItemKind` - #35728: Refactor `cx.pat_enum()` into `cx.pat_tuple_struct()` and `cx.pat_path()`. - #35850: Generalize the elements of lists in attributes from `MetaItem` to a new type `NestedMetaItem` that can represent a `MetaItem` or a literal. - #35917: Remove traits `AttrMetaMethods`, `AttributeMethods`, and `AttrNestedMetaItemMethods`. - Besides removing imports of these traits, this won't cause fallout. - Add a variant `Union` to `ItemKind` to future proof for `union` (c.f. #36016). - Remove inherent methods `attrs` and `fold_attrs` of `Annotatable`. - Use methods `attrs` and `map_attrs` of `HasAttrs` instead. r? @Manishearth

rust-highfive assigned sophiajt Aug 8, 2016

KiChjang reviewed Aug 8, 2016
View reviewed changes

KiChjang force-pushed the e0379-bonus branch 2 times, most recently from a552d03 to 7912ae2 Compare August 10, 2016 10:07

KiChjang changed the title ~~Move E0379 check from typeck to parser~~ Move E0379 check from typeck to ast validation Aug 10, 2016

nikomatsakis reviewed Aug 10, 2016
View reviewed changes

rust-highfive assigned nikomatsakis and unassigned sophiajt Aug 10, 2016

KiChjang force-pushed the e0379-bonus branch from 7912ae2 to d59fd35 Compare August 10, 2016 23:18

KiChjang force-pushed the e0379-bonus branch from af98e25 to 9c9994c Compare August 10, 2016 23:41

KiChjang force-pushed the e0379-bonus branch from 9c9994c to 7580188 Compare August 11, 2016 17:49

nikomatsakis reviewed Aug 17, 2016
View reviewed changes

KiChjang force-pushed the e0379-bonus branch from 49c5dcb to 9fae2f3 Compare August 17, 2016 21:10

KiChjang force-pushed the e0379-bonus branch from 9fae2f3 to 2659f3d Compare August 18, 2016 19:31

KiChjang force-pushed the e0379-bonus branch from 2659f3d to b5b667d Compare August 19, 2016 22:28

KiChjang added 3 commits August 27, 2016 22:43

Move E0379 check from typeck to ast validation

1e9e798

Change Constness to Spanned<Constness>

aa5c4bb

Add UI test for E0379

e46b09a

KiChjang force-pushed the e0379-bonus branch from b5b667d to e46b09a Compare August 28, 2016 05:43

jseyfried mentioned this pull request Aug 28, 2016

Batch up libsyntax breaking changes #36066

Merged

bors merged commit e46b09a into rust-lang:master Aug 30, 2016

KiChjang deleted the e0379-bonus branch August 30, 2016 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move E0379 check from typeck to ast validation #35480

Move E0379 check from typeck to ast validation #35480

KiChjang commented Aug 8, 2016

KiChjang Aug 8, 2016 •

edited

Loading

petrochenkov commented Aug 8, 2016

KiChjang commented Aug 10, 2016

KiChjang commented Aug 10, 2016

nikomatsakis Aug 10, 2016

petrochenkov Aug 10, 2016

KiChjang Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016 •

edited

Loading

KiChjang commented Aug 10, 2016

KiChjang commented Aug 10, 2016

nikomatsakis commented Aug 11, 2016

petrochenkov commented Aug 11, 2016

sophiajt commented Aug 11, 2016

KiChjang commented Aug 11, 2016

nikomatsakis commented Aug 11, 2016

nikomatsakis Aug 17, 2016

KiChjang commented Aug 17, 2016

nikomatsakis commented Aug 18, 2016

bors commented Aug 18, 2016

KiChjang commented Aug 18, 2016

sophiajt commented Aug 19, 2016

KiChjang commented Aug 19, 2016

sophiajt commented Aug 22, 2016

bors commented Aug 22, 2016

sophiajt commented Aug 22, 2016

bors commented Aug 22, 2016

bors commented Aug 22, 2016

jseyfried commented Aug 22, 2016

Move E0379 check from typeck to ast validation #35480

Move E0379 check from typeck to ast validation #35480

Conversation

KiChjang commented Aug 8, 2016

KiChjang Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

petrochenkov commented Aug 8, 2016

KiChjang commented Aug 10, 2016

KiChjang commented Aug 10, 2016

nikomatsakis Aug 10, 2016

Choose a reason for hiding this comment

petrochenkov Aug 10, 2016

Choose a reason for hiding this comment

KiChjang Aug 10, 2016

Choose a reason for hiding this comment

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016

nikomatsakis commented Aug 10, 2016 • edited Loading

KiChjang commented Aug 10, 2016

KiChjang commented Aug 10, 2016

nikomatsakis commented Aug 11, 2016

petrochenkov commented Aug 11, 2016

sophiajt commented Aug 11, 2016

KiChjang commented Aug 11, 2016

nikomatsakis commented Aug 11, 2016

nikomatsakis Aug 17, 2016

Choose a reason for hiding this comment

KiChjang commented Aug 17, 2016

nikomatsakis commented Aug 18, 2016

bors commented Aug 18, 2016

KiChjang commented Aug 18, 2016

sophiajt commented Aug 19, 2016

KiChjang commented Aug 19, 2016

sophiajt commented Aug 22, 2016

bors commented Aug 22, 2016

sophiajt commented Aug 22, 2016

bors commented Aug 22, 2016

bors commented Aug 22, 2016

jseyfried commented Aug 22, 2016

KiChjang Aug 8, 2016 •

edited

Loading

nikomatsakis commented Aug 10, 2016 •

edited

Loading