New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggest `..` for erroneous `...` in struct pattern; don't discard successfully-parsed fields on error #46721

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@zackmdavis
Member

zackmdavis commented Dec 14, 2017

full_field_parse

Regarding #46718.

r? @pnkfelix

zackmdavis added some commits Dec 14, 2017

don't throw away already-parsed field patterns on subsequent error
Previously, if an error occured while parsing field patterns, we would
just return the error, discarding any field patterns that did
successfully parse, which sometimes resulted in spurious diagnostics. At
the cost of a little boilerplate (the repetitious `.map_err(|err|
((fields.clone(), etc), err))` is annoying and the present author is
sorry to say she could not do better), we can pack the
successfully-parsed field patterns into the error and use them.

This was inspired by #46718.
@xfix

This comment has been minimized.

Contributor

xfix commented Dec 14, 2017

I wonder if it wouldn't be better to not print a message about missing field at all here. Using ... (even if it was supposed to be ..) sorta shows the intent to use struct update syntax, where missing fields aren't an issue.

@estebank

I like it. Agree with @xfix's suggestion but I'd accept a follow up PR for it.

let lo = self.span;
let hi;
if self.check(&token::DotDot) {
self.bump();
if self.token != token::CloseDelim(token::Brace) {
let token_str = self.this_token_to_string();
return Err(self.fatal(&format!("expected `{}`, found `{}`", "}",
token_str)))
let err = self.fatal(&format!("expected `{}`, found `{}`", "}",

This comment has been minimized.

@estebank

estebank Dec 14, 2017

Contributor

format!("expected {}...", "}", ...) is a bit silly, could you change it? :)

--> $DIR/issue-46718-expected-dotdot-found-dotdotdot.rs:21:13
|
21 | PersonalityInventory { expressivity: exp, ... } => exp
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing field `instrumentality`

This comment has been minimized.

@estebank

estebank Dec 14, 2017

Contributor

I agree with @xfix's comment, it'd be nice if this error wasn't shown. IIRC, this error happens during type check, so you'd have to modify the ast::StructPatKind and its equivalent in the hir to include a boolean recovered and only emit this error if it's true (I'm doing the same for type errors involving tail expressions and recovered if blocks in #46732). I'm sure that there're are other errors that could benefit from this signal too, but I wouldn't go looking to fix them on this PR. This change would probably cause many more files to be modified, so we could land this first and have a follow up PR if you prefer.

// accept trailing commas
if self.check(&token::CloseDelim(token::Brace)) { break }
}
let attrs = self.parse_outer_attributes()?;
let attrs = self.parse_outer_attributes()
.map_err(|err| ((fields.clone(), etc), err))?;
let lo = self.span;
let hi;
if self.check(&token::DotDot) {

This comment has been minimized.

@petrochenkov

petrochenkov Dec 14, 2017

Contributor

The only thing this PR needs to do is to replace this self.check(&token::DotDot) with self.check(&token::DotDot) || self.token == token::DotDotDot, and report a non-fatal error (self.span_err(...)) if it's actually a DotDotDot.

This comment has been minimized.

@zackmdavis

zackmdavis Dec 16, 2017

Member

I see ...

(I might claim that the extra machinery in this PR is still potentially useful for taking successfully-parsed field patterns into account when there's some other kind of syntax error ... but maybe that can be a separate, future PR if I find this PartialPResult device being useful elsewhere.)

@zackmdavis

This comment has been minimized.

Member

zackmdavis commented Dec 16, 2017

Closing in favor of simpler alternative #46763; thanks to @petrochenkov for the suggestion. (The PartialPResult machinery here could end up being incorporated in some future proposal, but it's overkill for #46721 alone.)

@zackmdavis zackmdavis closed this Dec 16, 2017

bors added a commit that referenced this pull request Dec 16, 2017

Auto merge of #46763 - zackmdavis:and_the_case_of_the_erroneous_field…
…_pattern_ellipsis, r=petrochenkov

in which `..` is suggested for erroneous `...` in struct field patterns

Resolves #46718. Supersedes #46721.

r? @petrochenkov

@zackmdavis zackmdavis deleted the zackmdavis:and_the_case_of_the_erroneous_ellipsis branch Jan 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment