Report more informative CSS errors by jdm · Pull Request #16752 · servo/servo

jdm · 2017-05-06T03:53:47Z

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.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

This change is

highfive · 2017-05-06T03:53:51Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/shorthand/inherited_svg.mako.rs, components/style/values/specified/calc.rs, components/style/values/specified/color.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs and 61 more
@KiChjang: components/script/dom/element.rs, components/script/dom/node.rs, components/script_layout_interface/reporter.rs
@fitzgen: components/script/dom/element.rs, components/script/dom/node.rs, components/script_layout_interface/reporter.rs
@emilio: components/style/properties/shorthand/inherited_svg.mako.rs, components/style/values/specified/calc.rs, components/style/values/specified/color.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs and 62 more

highfive · 2017-05-06T03:53:52Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

nox · 2017-05-06T09:18:02Z

components/style/error_reporting.rs

+#[cfg(feature = "gecko")]
+pub fn create_error_reporter() -> RustLogReporter {
+    RustLogReporter
+}


Aren't both of them the exact same function?

Yes. That changes in my patches to Gecko.

ghost · 2017-05-06T09:25:26Z

cc @servo/style

emilio

Wow, that's a big diff. Haven't gone through it with detail, would love help from @nox/@SimonSapin reviewing.

Some questions below.

emilio · 2017-05-06T16:24:55Z

components/script_layout_interface/reporter.rs

                  line_offset,
                  location.column,
-                  message)
+                  error.to_string())


Perhaps we should implement Display for ContextualParseError? Fine as a followup I guess.

emilio · 2017-05-06T16:26:45Z

components/selectors/parser.rs

+pub enum SelectorParseError<T> {
+    PseudoElementInComplexSelector,
+    NoQualifiedNameInAttributeSelector,
+    TooManyCompoundSelectorComponents,


Let's rename this to TooManyCompoundSelectorComponentsInNegation, or perhaps TooComplexNegationSelector?

emilio · 2017-05-06T16:29:50Z

components/style/error_reporting.rs

+
+impl<'a> ContextualParseError<'a> {
+    /// Turn a parse error into a string representation.
+    pub fn to_string(&self) -> String {


Perhaps we should implement Display and implement to_string as format!("{}", self)? Fine also as a followup/easy issue I guess :)

emilio · 2017-05-06T16:31:26Z

components/style/properties/longhand/text.mako.rs

+            let result: Result<_, ParseError> = input.try(|input| {
+                match input.expect_ident() {
+                    Ok(ident) => {
+                        (match_ignore_ascii_case! { &ident,


I think it shouldn't be needed to wrap it in parenthesis anymore?

Fixing this will be kind of annoying given how often this pattern appears. I would prefer not to, if that's alright.

Yeah, that's totally fine for me :)

emilio · 2017-05-06T16:32:35Z

components/style/values/generics/position.rs

            if let Ok(l) = input.try(|i| L::parse(context, i)) {
                if pos.is_some() {
-                    return Err(())
+                    return Err(StyleParseError::UnspecifiedError.into())


Is there an intention to convert all the UnspecifiedErrors into more explicit error kinds as followup work?

Yes. It's a nice source of easy issues and a lot of effort for this patch in particular.

emilio · 2017-05-06T16:33:37Z

components/style/values/mod.rs

+            _ => true
        };
+        if !valid {
+            return Err(BasicParseError::UnexpectedToken(Token::Ident(ident)).into());


Perhaps just use braces in the upper match branch and get rid of valid?

nvm about this one.

emilio · 2017-05-06T16:35:13Z

components/style/values/specified/align.rs

        "stretch" => Ok(ALIGN_STRETCH),
        _ => Err(())
-    }
+    }).map_err(|()| BasicParseError::UnexpectedToken(Token::Ident(ident)).into())


I'm assuming this funky way of doing this is because ident is borrowed in the match expression, is that right? If so, that's kind of unfortunate, I guess... :(

Yep! Best compromise I could find.

emilio · 2017-05-06T16:37:05Z

components/style/values/specified/basic_shape.rs

            Ok(ref s) if s.eq_ignore_ascii_case("inset") =>
                input.parse_nested_block(|i| GenericInsetRect::parse_function_arguments(context, i)),
-            _ => Err(())
+            _ => Err(BasicParseError::ExpectedToken(Token::Function("inset".into())).into())


Do you know if this "inset".into() will allocate or not? I'm assuming that since it's an static string it shouldn't need to, but I worry that perhaps to coerce it to do that you may need to be explicit about it. Not a big deal in any case I think

It should be a Cow::Borrowed afaik.

bors-servo · 2017-05-06T21:54:50Z

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

SimonSapin · 2017-05-19T07:13:52Z

Reviewed 9 of 9 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, 75 of 75 files at r4.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

components/style/custom_properties.rs, line 241 at r4 (raw file):

                return Err(StyleParseError::CloseSquareBracketInDeclarationValueBlock.into()),
            Token::CloseCurlyBracket =>
                return Err(StyleParseError::CloseCurlyBracketInDeclarationValueBlock.into()),

These are not just (various kinds of) closing brackets, they are unbalanced / improperly nested ones.

components/style/media_queries.rs, line 227 at r4 (raw file):

            Ok(ident) => {
                let result: Result<_, ParseError> = MediaQueryType::parse(&*ident)
                    .map_err(|()| BasicParseError::UnexpectedToken(Token::Ident(ident)).into());

Could you avoid creating tokens like this? I have plans to change the Token data structure (to avoid more memory allocations) that involve structs with private fields, so that only the tokenizer will be able to create tokens. Maybe add a UnexpectedIdent error variant instead?

Comments from Reviewable

jdm · 2017-06-07T19:15:20Z

@SimonSapin I have addressed the review comments and made a new commit which applies to the existing code that you reviewed. I'm going to go ahead and rebase all of my changes separately now, and I won't push them until you are done reviewing the existing code.

SimonSapin · 2017-06-07T21:05:39Z

r+

Reviewed 54 of 54 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

components/style/values/specified/basic_shape.rs, line 131 at r4 (raw file):

Previously, jdm (Josh Matthews) wrote…

It should be a Cow::Borrowed afaik.

Indeed, this uses Cow::Borrowed and does not allocate.

Comments from Reviewable

jdm · 2017-06-09T19:25:25Z

@bors-servo: try

bors-servo · 2017-06-09T19:25:29Z

⌛ Trying commit 98d0fb2 with merge d6288b9...

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)

bors-servo · 2017-06-09T19:49:48Z

💔 Test failed - windows-msvc-dev

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)

jdm · 2017-06-09T20:47:09Z

@bors-servo: r=SimonSapin

bors-servo · 2017-06-09T20:47:09Z

📌 Commit 27ae1ef has been approved by SimonSapin

jdm · 2017-06-09T20:47:15Z

@bors-servo: p=1

bors-servo · 2017-06-09T21:31:51Z

⌛ Testing commit 27ae1ef with merge 061cb5f...

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)

bors-servo · 2017-06-09T22:56:07Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: SimonSapin
Pushing 061cb5f to master...

nox · 2017-06-10T11:15:19Z

@jdm What was the problem with not sharing the same ParseError across all impls again?

nox · 2017-06-10T11:22:51Z

components/style/properties/longhand/background.mako.rs

+            let horizontal = try!(horizontal);
+            let vertical = input.try(RepeatKeyword::parse).ok();
+            Ok(SpecifiedValue::Other(horizontal, vertical))
+        })


@jdm Why was that change needed?

Oh yeah borrowing issues, unfortunate. :(

jdm · 2017-06-11T04:58:17Z

@nox I'm not sure I understand the question. If it is why rust-cssparser's ParseError does not contain every possible error, the answer is because that's really inconvenient API design.

jdm added the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label May 6, 2017

highfive assigned ghost May 6, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 6, 2017

jdm mentioned this pull request May 6, 2017

Return more specific errors from parsing APIs servo/rust-cssparser#143

Merged

jdm force-pushed the css-parse-error branch from c013f80 to 8d26cb8 Compare May 6, 2017 04:38

nox reviewed May 6, 2017

View reviewed changes

emilio reviewed May 6, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label May 6, 2017

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. labels May 19, 2017

jdm mentioned this pull request Jun 7, 2017

Return meaningful error values when parsing servo/rust-cssparser#155

Merged

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 7, 2017

jdm force-pushed the css-parse-error branch from 6c3f4e4 to f9018ab Compare June 9, 2017 17:13

jdm added 3 commits June 9, 2017 13:16

Report CSS parse errors via enum instead of strings.

fd6e54d

Encapsulate CSS error reporter creation for stylo.

3773a4d

The ParseErrorReporter trait no longer needs to be Send.

58e39bf

jdm force-pushed the css-parse-error branch 2 times, most recently from bedea39 to 98d0fb2 Compare June 9, 2017 19:25

jdm removed the S-needs-rebase There are merge conflict errors. label Jun 9, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 9, 2017

jdm force-pushed the css-parse-error branch from 98d0fb2 to aceffc5 Compare June 9, 2017 20:24

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 9, 2017

Thread ParseError return values through CSS parsing.

27ae1ef

jdm force-pushed the css-parse-error branch from aceffc5 to 27ae1ef Compare June 9, 2017 20:46

highfive assigned SimonSapin and unassigned ghost Jun 9, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 9, 2017

bors-servo mentioned this pull request Jun 9, 2017

Bump euclid to 0.14.x. #17184

Merged

5 tasks

bors-servo merged commit 27ae1ef into servo:master Jun 9, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 9, 2017

nox reviewed Jun 10, 2017

View reviewed changes

Uh oh!

Conversation

jdm commented May 6, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented May 6, 2017

Uh oh!

highfive commented May 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 6, 2017

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented May 6, 2017

Uh oh!

SimonSapin commented May 19, 2017

Uh oh!

jdm commented Jun 7, 2017

Uh oh!

SimonSapin commented Jun 7, 2017

Uh oh!

jdm commented Jun 9, 2017

Uh oh!

bors-servo commented Jun 9, 2017

Uh oh!

bors-servo commented Jun 9, 2017

Uh oh!

jdm commented Jun 9, 2017

Uh oh!

bors-servo commented Jun 9, 2017

Uh oh!

jdm commented Jun 9, 2017

Uh oh!

bors-servo commented Jun 9, 2017

Uh oh!

bors-servo commented Jun 9, 2017

Uh oh!

nox commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm commented Jun 11, 2017

Uh oh!

Reviewers

Assignees

jdm commented May 6, 2017 •

edited by larsbergstrom

Loading