Skip to content

Commit

Permalink
style: Centralize a bit invalid value error reporting.
Browse files Browse the repository at this point in the history 
Also, buffer the errors, since we're going to want to look at the whole
declaration block to skip reporting them.

This shouldn't change behavior, just moves some work to the caller, and defers a
bit the work so that it happens only when error reporting is enabled.

Differential Revision: https://phabricator.services.mozilla.com/D30200
  • Loading branch information
@emilio
emilio committed May 10, 2019
1 parent 29cecf6 commit 0221026
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 43 deletions.
6 changes: 6 additions & 0 deletions components/style/parser.rs
View file
Expand Up @@ -136,6 +136,12 @@ impl<'a> ParserContext<'a> {
.expect("Rule type expected, but none was found.")
}

/// Returns whether CSS error reporting is enabled.
#[inline]
pub fn error_reporting_enabled(&self) -> bool {
self.error_reporter.is_some()
}

/// Record a CSS parse error with this context’s error reporting.
pub fn log_css_error(&self, location: SourceLocation, error: ContextualParseError) {
let error_reporter = match self.error_reporter {
Expand Down
90 changes: 74 additions & 16 deletions components/style/properties/declaration_block.rs
View file
Expand Up @@ -1240,26 +1240,37 @@ pub fn parse_one_declaration_into(
None,
);

let property_id_for_error_reporting = if context.error_reporting_enabled() {
Some(id.clone())
} else {
None
};

let mut input = ParserInput::new(input);
let mut parser = Parser::new(&mut input);
let start_position = parser.position();
parser.parse_entirely(|parser| {
PropertyDeclaration::parse_into(declarations, id, &context, parser)
}).map_err(|err| {
let location = err.location;
let error = ContextualParseError::UnsupportedPropertyDeclaration(
parser.slice_from(start_position),
err,
None
);
context.log_css_error(location, error);
if context.error_reporting_enabled() {
report_one_css_error(
&context,
None,
None,
err,
parser.slice_from(start_position),
property_id_for_error_reporting,
)
}
})
}

/// A struct to parse property declarations.
struct PropertyDeclarationParser<'a, 'b: 'a> {
context: &'a ParserContext<'b>,
declarations: &'a mut SourcePropertyDeclaration,
/// The last parsed property id if non-custom, and if any.
last_parsed_property_id: Option<PropertyId>,
}


Expand Down Expand Up @@ -1296,6 +1307,9 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
}));
}
};
if self.context.error_reporting_enabled() {
self.last_parsed_property_id = Some(id.clone());
}
input.parse_until_before(Delimiter::Bang, |input| {
PropertyDeclaration::parse_into(self.declarations, id, self.context, input)
})?;
Expand All @@ -1309,6 +1323,48 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
}
}

type SmallParseErrorVec<'i> = SmallVec<[(ParseError<'i>, &'i str, Option<PropertyId>); 2]>;

#[cold]
fn report_one_css_error<'i>(
context: &ParserContext,
_block: Option<&PropertyDeclarationBlock>,
selectors: Option<&SelectorList<SelectorImpl>>,
mut error: ParseError<'i>,
slice: &str,
property: Option<PropertyId>,
) {
debug_assert!(context.error_reporting_enabled());

// If the unrecognized property looks like a vendor-specific property,
// silently ignore it instead of polluting the error output.
if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
return;
}

if let Some(ref property) = property {
error = match *property {
PropertyId::Custom(ref c) => StyleParseErrorKind::new_invalid(format!("--{}", c), error),
_ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error),
};
}

let location = error.location;
let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
context.log_css_error(location, error);
}

#[cold]
fn report_css_errors(
context: &ParserContext,
block: &PropertyDeclarationBlock,
selectors: Option<&SelectorList<SelectorImpl>>,
errors: &mut SmallParseErrorVec,
) {
for (mut error, slice, property) in errors.drain() {
report_one_css_error(context, Some(block), selectors, error, slice, property)
}
}

/// Parse a list of property declarations and return a property declaration
/// block.
Expand All @@ -1320,10 +1376,12 @@ pub fn parse_property_declaration_list(
let mut declarations = SourcePropertyDeclaration::new();
let mut block = PropertyDeclarationBlock::new();
let parser = PropertyDeclarationParser {
context: context,
context,
last_parsed_property_id: None,
declarations: &mut declarations,
};
let mut iter = DeclarationListParser::new(input, parser);
let mut errors = SmallParseErrorVec::new();
while let Some(declaration) = iter.next() {
match declaration {
Ok(importance) => {
Expand All @@ -1335,17 +1393,17 @@ pub fn parse_property_declaration_list(
Err((error, slice)) => {
iter.parser.declarations.clear();

// If the unrecognized property looks like a vendor-specific property,
// silently ignore it instead of polluting the error output.
if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
continue;
if context.error_reporting_enabled() {
let property = iter.parser.last_parsed_property_id.take();
errors.push((error, slice, property));
}

let location = error.location;
let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
context.log_css_error(location, error);
}
}
}

if !errors.is_empty() {
report_css_errors(context, &block, selectors, &mut errors)
}

block
}
34 changes: 7 additions & 27 deletions components/style/properties/properties.mako.rs
View file
Expand Up @@ -2209,13 +2209,9 @@ impl PropertyDeclaration {
// This probably affects some test results.
let value = match input.try(CSSWideKeyword::parse) {
Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword),
Err(()) => match crate::custom_properties::SpecifiedValue::parse(input) {
Ok(value) => CustomDeclarationValue::Value(value),
Err(e) => return Err(StyleParseErrorKind::new_invalid(
format!("--{}", property_name),
e,
)),
}
Err(()) => CustomDeclarationValue::Value(
crate::custom_properties::SpecifiedValue::parse(input)?
),
};
declarations.push(PropertyDeclaration::Custom(CustomDeclaration {
name: property_name,
Expand All @@ -2236,19 +2232,11 @@ impl PropertyDeclaration {
.or_else(|err| {
while let Ok(_) = input.next() {} // Look for var() after the error.
if !input.seen_var_or_env_functions() {
return Err(StyleParseErrorKind::new_invalid(
non_custom_id.unwrap().name(),
err,
));
return Err(err);
}
input.reset(&start);
let (first_token_type, css) =
crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
StyleParseErrorKind::new_invalid(
non_custom_id.unwrap().name(),
e,
)
})?;
crate::custom_properties::parse_non_custom_with_var(input)?;
Ok(PropertyDeclaration::WithVariables(VariableDeclaration {
id,
value: Arc::new(UnparsedValue {
Expand Down Expand Up @@ -2287,20 +2275,12 @@ impl PropertyDeclaration {
id.parse_into(declarations, context, input).or_else(|err| {
while let Ok(_) = input.next() {} // Look for var() after the error.
if !input.seen_var_or_env_functions() {
return Err(StyleParseErrorKind::new_invalid(
non_custom_id.unwrap().name(),
err,
));
return Err(err);
}

input.reset(&start);
let (first_token_type, css) =
crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
StyleParseErrorKind::new_invalid(
non_custom_id.unwrap().name(),
e,
)
})?;
crate::custom_properties::parse_non_custom_with_var(input)?;
let unparsed = Arc::new(UnparsedValue {
css: css.into_owned(),
first_token_type: first_token_type,
Expand Down

0 comments on commit 0221026

Please sign in to comment.