Report more informative CSS errors #16752

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.

wafflespeanut · 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:40:39Z

components/script_layout_interface/reporter.rs

        let location = input.source_location(position);
        let line_offset = location.line + line_number_offset as usize;
        if log_enabled!(log::LogLevel::Info) {
            info!("Url:\t{}\n{}:{} {}",
                  url.as_str(),
                  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:40:39Z

components/selectors/parser.rs

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


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

emilio · 2017-05-06T16:40:39Z

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:40:39Z

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:40:39Z

components/style/values/generics/position.rs

        where F: FnMut(Keyword) -> bool
    {
        let (mut pos, mut keyword) = (None, None);
        for _ in 0..2 {
            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:40:39Z

components/style/values/mod.rs

        };
+        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:40:39Z

components/style/values/specified/align.rs

        "auto" => Ok(ALIGN_AUTO),
        "normal" => Ok(ALIGN_NORMAL),
        "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:40:39Z

components/style/values/specified/basic_shape.rs

        match input.try(|i| i.expect_function()) {
            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


        Report CSS parse errors via enum instead of strings.


        Encapsulate CSS error reporter creation for stylo.


        The ParseErrorReporter trait no longer needs to be Send.

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

@bors-servo: try

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

⌛ Trying commit 98d0fb2 with merge d6288b9...


        Auto merge of #16752 - jdm:css-parse-error, r=<try>

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


        Auto merge of #155 - jdm:err2, r=SimonSapin

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)


        Thread ParseError return values through CSS parsing.

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...


        Auto merge of #16752 - jdm:css-parse-error, r=SimonSapin

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 label May 6, 2017

highfive assigned wafflespeanut May 6, 2017

highfive added the S-awaiting-review 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 jdm:css-parse-error branch from c013f80 to 8d26cb8 May 6, 2017

nox reviewed May 6, 2017

View changes

emilio reviewed May 6, 2017

View changes

highfive added the S-needs-rebase label May 6, 2017

jdm added S-needs-code-changes and removed S-awaiting-review S-blocked-on-external 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 and removed S-needs-code-changes labels Jun 7, 2017

jdm force-pushed the jdm:css-parse-error branch from 6c3f4e4 to f9018ab Jun 9, 2017

jdm added 3 commits Apr 18, 2017

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 jdm:css-parse-error branch 2 times, most recently from bedea39 to 98d0fb2 Jun 9, 2017

jdm removed the S-needs-rebase label Jun 9, 2017

highfive added the S-tests-failed label Jun 9, 2017

jdm force-pushed the jdm:css-parse-error branch from 98d0fb2 to aceffc5 Jun 9, 2017

highfive removed the S-tests-failed label Jun 9, 2017

Thread ParseError return values through CSS parsing.

Loading status checks…

27ae1ef

jdm force-pushed the jdm:css-parse-error branch from aceffc5 to 27ae1ef Jun 9, 2017

highfive assigned SimonSapin and unassigned wafflespeanut Jun 9, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 9, 2017

bors-servo mentioned this pull request Jun 9, 2017

Bump euclid to 0.14.x. #17184

Merged

3 of 5 tasks complete

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 9, 2017

nox reviewed Jun 10, 2017

View changes

servo / servo

Report more informative CSS errors #16752

Report more informative CSS errors #16752

jdm commented May 6, 2017 •

edited by larsbergstrom

highfive commented May 6, 2017

highfive commented May 6, 2017

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented May 6, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 6, 2017

SimonSapin commented May 19, 2017

jdm commented Jun 7, 2017

SimonSapin commented Jun 7, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

nox commented Jun 10, 2017

This comment has been minimized.

This comment has been minimized.

jdm commented Jun 11, 2017

servo / servo

Join GitHub today

Report more informative CSS errors #16752

Report more informative CSS errors #16752

Conversation

jdm commented May 6, 2017 • edited by larsbergstrom

highfive commented May 6, 2017

highfive commented May 6, 2017

This comment has been minimized.

This comment has been minimized.

wafflespeanut commented May 6, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 6, 2017

SimonSapin commented May 19, 2017

jdm commented Jun 7, 2017

SimonSapin commented Jun 7, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

jdm commented Jun 9, 2017

bors-servo commented Jun 9, 2017

bors-servo commented Jun 9, 2017

nox commented Jun 10, 2017

This comment has been minimized.

This comment has been minimized.

jdm commented Jun 11, 2017

jdm commented May 6, 2017 •

edited by larsbergstrom