Return more specific errors from parsing APIs #143

jdm · 2017-05-05T01:07:06Z

This shouldn't be merged until the related Servo and Gecko work is complete, but I figured I would put this up for feedback.

This change is

jdm · 2017-05-06T03:54:13Z

Since servo/servo#16752 is ready for review, please review this!

emilio

This looks overall pretty great to me.

I'm curious about which non-UnexpectedEof errors you're seeing coming from Tokenizer::next (what other errors can it possibly return?), and I have a few minor nits around.

Probably @SimonSapin wants to take a look, since he was looking into parsing performance and this has a potential impact on that (though probably not too much).

I also wonder if we could (ab)use the UnexpectedToken errors to do a bit less tokenization in Servo...

Awesome job overall!

emilio · 2017-05-06T16:18:19Z

src/parser.rs

-            Ok(_) => {
-                Err(())
-            }
+            //FIXME: swallowing non-UnexpectedEof errors seems wrong, but tests fail otherwise.


Huh... Could you file an issue with more details, and perhaps an example, and reference it here, just to see if it's something expected or not? Which other errors are reported?

(also, uber-nit: a space after //)

Filed #144.

I took a look at #144, It's legit and we should remove the fixme and perhaps add a comment there.

emilio · 2017-05-06T16:20:12Z

src/rules_and_declarations.rs

@@ -52,6 +52,9 @@ pub trait DeclarationParser {
    /// The finished representation of a declaration.
    type Declaration;

+    ///


Missing docs? :)

emilio · 2017-05-06T16:20:21Z

src/rules_and_declarations.rs

@@ -89,6 +93,9 @@ pub trait AtRuleParser {
    /// The finished representation of an at-rule.
    type AtRule;

+    ///
+    type Error;


Please document it too

emilio · 2017-05-06T16:20:44Z

src/rules_and_declarations.rs

@@ -266,8 +284,8 @@ pub struct RuleListParser<'i: 't, 't: 'a, 'a, P> {
 }


-impl<'i: 't, 't: 'a, 'a, R, P> RuleListParser<'i, 't, 'a, P>
-where P: QualifiedRuleParser<QualifiedRule = R> + AtRuleParser<AtRule = R> {
+impl<'i: 't, 't: 'a, 'a, R, P, E> RuleListParser<'i, 't, 'a, P>


Wow, those are a lot of lifetimes.

jdm · 2017-05-08T17:34:22Z

Review comments addressed. @SimonSapin when do you think you will be able to look at this?

emilio · 2017-05-09T01:01:13Z

src/rules_and_declarations.rs

@@ -157,6 +164,9 @@ pub trait QualifiedRuleParser {
    /// The finished representation of a qualified rule.
    type QualifiedRule;

+    ///


This one seems to still be missing. A copy of the other Error docs is probably ok.

SimonSapin · 2017-05-10T22:24:46Z

IRC conversation:

SimonSapin> jdm: looking at Return more specific errors from parsing APIs #143 , I don’t have a precise counter-proposal but I think the set of error types don’t quite make sense

jdm> go on

SimonSapin> jdm: for PreciseParseError should be the basic thing

SimonSapin> jdm: and the generic ParseError::Custom(T) feels like the wrong pattern

jdm isn't sure how else to incorporate errors from outside of the library

SimonSapin> jdm: the Basic / Custom enum should not exist, instead things should use From conversions as needed

jdm> conversions into what?

jdm> SimonSapin: keep in mind that BasicParseError exists as a separate type to reduce the number of explicit type annotations that are required

jdm> SimonSapin: when I threaded ParseError through the whole API it because extremely frustrating to use

jdm> so for the APIs that can't return custom types, it's much easier to return a small set of known errors

SimonSapin> jdm: for example instead of returning Result <T, ParseError<'i, E>>, parse_entirely could return Result<T, E> where E: From<RemainingInput>

SimonSapin> or E: From<ParseError>

jdm> see, that would lead to a lot of ambugious types that would need resolving

jdm> and that becomes very gross

jdm> actually I might be misinterpreting that

jdm> SimonSapin: what is RemainingInput?

SimonSapin> jdm: an hypothetical unit type

jdm> hmm

jdm> SimonSapin: ok, I have a minimal form of selectors and style that I can prototype changes to rust-cssparser against

jdm> SimonSapin: so I'll try that out

SimonSapin> jdm: minimal form?

jdm> SimonSapin: yeah; it has parsing code for background-color, custom properties, and something that uses the selectors/ parsing traits

jdm> SimonSapin: rather than having to redo the return types for about 3000 parsing functions

SimonSapin> interesting

SimonSapin> jdm: alternative idea: no generic E, one ParseError type to rule them all. Add variants/fields to it as needed. For example &'static str that describes what was expected instead of that unexpected token

jdm> SimonSapin: could you record this conversation in the PR, please?

SimonSapin> sure

jdm> SimonSapin: I don't think the one type to rule them all makes sense given how many errors will be added to style/

SimonSapin> jdm: I haven’t looked at the servo PR yet. Are there many error types? Why?

jdm> SimonSapin: because we want to provide enough information for gecko to use https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/layout/css.properties

jdm> SimonSapin: the servo PR provides useful context

jdm> (for understanding the rust-cssparser changes)

SimonSapin> jdm: do we need this exact set of error cases, or just providing roughly as useful info?

jdm> SimonSapin: hard to say; I'm aiming for more information than my first attempt could provide.

jdm> which was basically nothing more than "this rule was invalid"

bors-servo · 2017-05-14T09:25:03Z

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

jdm · 2017-05-16T19:57:39Z

I've spent some time trying to integrate your ideas about returning E where E: From<...> (I've started out with From<BasicParseError<'i>> as something concrete), and I'm really having trouble making any traction. Between issues with incorporating lifetimes (such as the error types in the traits in rules_and_declarations.rs) and type-inference errors when there are too many layers of generics, I don't see a clear path forward. I would like to move forward with the existing design that actually compiles unless you have a concrete counter-proposal.

SimonSapin · 2017-05-18T14:53:56Z

I’m still convinced that the generic enum ParseError<T> { …, Custom(T) } is not a good pattern, but at the same time if I spent a bunch of time to come up with a concrete counter-proposal I’m likely to hit the same issues as you did and I don’t know that I’ll have any better solution. So let’s call this r+ on the general approach.

Reviewed 5 of 7 files at r2, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

src/parser.rs, line 373 at r4 (raw file):

        let byte = self.tokenizer.next_byte();
        if self.stop_before.contains(Delimiters::from_byte(byte)) {
            return Err(BasicParseError::UnexpectedToken(Token::Delim(byte.unwrap_or(0) as char)))

Returning UnexpectedToken here is incorrect. The point of self.stop_before is to provide the abstraction that we’re parsing a subslice of the original stream. Finding the byte/token that marks the end of that sub-stream is the expected end of the stream.

Maybe UnexpectedEof should be renamed EndOfInput?

src/parser.rs, line 610 at r4 (raw file):

        match token {
            Token::Number(NumericValue { ref int_value, .. }) if int_value.is_some() => {
                Ok(int_value.unwrap())

Please use int_value: Some(int_value) in the pattern here to make unwrap unnecessary.

Comments from Reviewable

nox · 2017-05-18T15:04:14Z

I don't like this design either.

What I would have expected is for cssparser to only define a basic set of errors, and for the Parse trait in Servo to have an associated type Error, like Serde's Serializer trait.

This way each specific parser in Servo would care only about its specific errors, and we wouldn't need to have all errors possible in Servo in a single huge enum.

SimonSapin · 2017-05-18T15:08:42Z

I suspect that reconciling lifetime parameters with generic error types would require "associated type constructors" rust-lang/rfcs#1598

nox · 2017-05-18T15:09:37Z

I see.

SimonSapin · 2017-05-18T15:11:29Z

@bors-servo r+

Reviewed 8 of 8 files at r6.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

bors-servo · 2017-05-18T15:11:33Z

📌 Commit 40d86b0 has been approved by SimonSapin

bors-servo · 2017-05-18T15:11:37Z

⌛ Testing commit 40d86b0 with merge 0a70d22...

Return more specific errors from parsing APIs This shouldn't be merged until the related Servo and Gecko work is complete, but I figured I would put this up for feedback.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/143)

bors-servo · 2017-05-18T15:16:40Z

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing 0a70d22 to master...

This reverts commit 0a70d22, reversing changes made to fc0bdcd.

Report more informative CSS errors This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16752)

Return meaningful error values when parsing This is #143 with an extra commit added to accommodate changes requested in servo/servo#16752.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/155)

Report more informative CSS errors This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16752)

…s-parse-error); r=SimonSapin This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: 061cb5f48e5c93a5decf39e530aea4a566e97341 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 683cf352b472110df4b28c191e8850763334134d

…s-parse-error); r=SimonSapin This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: 061cb5f48e5c93a5decf39e530aea4a566e97341

…s-parse-error); r=SimonSapin This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: 061cb5f48e5c93a5decf39e530aea4a566e97341 UltraBlame original commit: 79d8f06313932e8f116100390dd153a1c8f9734b

…s-parse-error); r=SimonSapin This requires servo/rust-cssparser#143 for the final commit. There's no better way to split that work up, unfortunately, and it's extremely easy to bitrot. I would appreciate if we could expedite reviewing this work. This is the work necessary to enable https://bugzilla.mozilla.org/show_bug.cgi?id=1352669. It makes sense to merge it separately because it's so much effort to keep it up to date with the ongoing Stylo work. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: 061cb5f48e5c93a5decf39e530aea4a566e97341

jdm mentioned this pull request May 6, 2017

Report more informative CSS errors servo/servo#16752

Merged

3 tasks

emilio reviewed May 6, 2017

View reviewed changes

jdm mentioned this pull request May 8, 2017

Parser::expect_exhausted swallows non-UnexpectedEof errors #144

Closed

jdm force-pushed the parseerror branch from 458aaff to 18f7bcc Compare May 8, 2017 17:33

emilio reviewed May 9, 2017

View reviewed changes

jdm force-pushed the parseerror branch from 18f7bcc to 1b8dcea Compare May 9, 2017 01:22

jdm added 2 commits May 18, 2017 11:06

Return more specific parse error data.

04bef8c

Make parse error types extensible.

40d86b0

jdm force-pushed the parseerror branch from 1b8dcea to 40d86b0 Compare May 18, 2017 15:08

bors-servo merged commit 40d86b0 into servo:master May 18, 2017

jdm added a commit to jdm/rust-cssparser that referenced this pull request May 18, 2017

Revert "Auto merge of servo#143 - jdm:parseerror, r=SimonSapin"

8ac1afd

This reverts commit 0a70d22, reversing changes made to fc0bdcd.

jdm mentioned this pull request Jun 7, 2017

Return meaningful error values when parsing #155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return more specific errors from parsing APIs #143

Return more specific errors from parsing APIs #143

jdm commented May 5, 2017 •

edited by larsbergstrom

Loading

jdm commented May 6, 2017

emilio left a comment

emilio May 6, 2017

jdm May 8, 2017

emilio May 9, 2017

emilio May 6, 2017

emilio May 6, 2017

emilio May 6, 2017

jdm commented May 8, 2017

emilio May 9, 2017

SimonSapin commented May 10, 2017 •

edited by jdm

Loading

bors-servo commented May 14, 2017

jdm commented May 16, 2017

SimonSapin commented May 18, 2017 •

edited

Loading

nox commented May 18, 2017

SimonSapin commented May 18, 2017

nox commented May 18, 2017

SimonSapin commented May 18, 2017

bors-servo commented May 18, 2017

bors-servo commented May 18, 2017

bors-servo commented May 18, 2017

Return more specific errors from parsing APIs #143

Return more specific errors from parsing APIs #143

Conversation

jdm commented May 5, 2017 • edited by larsbergstrom Loading

jdm commented May 6, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio May 6, 2017

Choose a reason for hiding this comment

jdm May 8, 2017

Choose a reason for hiding this comment

emilio May 9, 2017

Choose a reason for hiding this comment

emilio May 6, 2017

Choose a reason for hiding this comment

emilio May 6, 2017

Choose a reason for hiding this comment

emilio May 6, 2017

Choose a reason for hiding this comment

jdm commented May 8, 2017

emilio May 9, 2017

Choose a reason for hiding this comment

SimonSapin commented May 10, 2017 • edited by jdm Loading

bors-servo commented May 14, 2017

jdm commented May 16, 2017

SimonSapin commented May 18, 2017 • edited Loading

nox commented May 18, 2017

SimonSapin commented May 18, 2017

nox commented May 18, 2017

SimonSapin commented May 18, 2017

bors-servo commented May 18, 2017

bors-servo commented May 18, 2017

bors-servo commented May 18, 2017

jdm commented May 5, 2017 •

edited by larsbergstrom

Loading

SimonSapin commented May 10, 2017 •

edited by jdm

Loading

SimonSapin commented May 18, 2017 •

edited

Loading