Skip to content

Commit

Permalink
Add a ParenthesizedExpression class (#503)
Browse files Browse the repository at this point in the history
This allows us to accurately track the source spans for parenthesized
expressions, which in turn allows us to print accurate error
indications.

Adding a new class for this more accurately represents the structure
of the expression, but it also involves an extra allocation during
parsing and an extra level of nesting during evaluation which could
have a small but real performance impact.

We could alternatively add a package-internal setter for
Expression.span, and update the source span for parenthesized
expressions after they're initially parsed. However, this has its own
downsides: it adds complexity and mutability to the object model; and
many expression classes currently use lazily-generated spans, so
making them settable would require adding extra slots on those
classes.

I decided to go with the extra class because it only adds overhead
when parentheses are actually used in practice, as opposed to adding
overhead to every list/color/etc. The runtime overhead is also likely
to be mitigated if at any point we add a constant-folding step.
  • Loading branch information
nex3 committed Oct 17, 2018
1 parent 96468e4 commit a25bbb3
Show file tree
Hide file tree
Showing 9 changed files with 56 additions and 3 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,9 @@
* When running in compressed mode, remove spaces around combinators in complex
selectors, so a selector like `a > b` is output as `a>b`.

* Properly indicate the source span for errors involving binary operation
expressions whose operands are parenthesized.

## 1.14.2

* Fix a bug where loading the same stylesheet from two different import paths
Expand Down
1 change: 1 addition & 0 deletions lib/src/ast/sass.dart
Expand Up @@ -17,6 +17,7 @@ export 'sass/expression/list.dart';
export 'sass/expression/map.dart';
export 'sass/expression/null.dart';
export 'sass/expression/number.dart';
export 'sass/expression/parenthesized.dart';
export 'sass/expression/selector.dart';
export 'sass/expression/string.dart';
export 'sass/expression/unary_operation.dart';
Expand Down
24 changes: 24 additions & 0 deletions lib/src/ast/sass/expression/parenthesized.dart
@@ -0,0 +1,24 @@
// Copyright 2018 Google Inc. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import 'package:source_span/source_span.dart';

import '../../../value.dart';
import '../../../visitor/interface/expression.dart';
import '../expression.dart';

/// An expression wrapped in parentheses.
class ParenthesizedExpression implements Expression {
/// The internal expression.
final Expression expression;

final FileSpan span;

ParenthesizedExpression(this.expression, this.span);

T accept<T>(ExpressionVisitor<T> visitor) =>
visitor.visitParenthesizedExpression(this);

String toString() => expression.toString();
}
2 changes: 1 addition & 1 deletion lib/src/parse/stylesheet.dart
Expand Up @@ -1772,7 +1772,7 @@ relase. For details, see http://bit.ly/moz-document.

if (!scanner.scanChar($comma)) {
scanner.expectChar($rparen);
return first;
return new ParenthesizedExpression(first, scanner.spanFrom(start));
}
whitespace();

Expand Down
3 changes: 3 additions & 0 deletions lib/src/visitor/async_evaluate.dart
Expand Up @@ -1252,6 +1252,9 @@ class _EvaluateVisitor
Future<SassNumber> visitNumberExpression(NumberExpression node) async =>
new SassNumber(node.value, node.unit);

Future<Value> visitParenthesizedExpression(ParenthesizedExpression node) =>
node.expression.accept(this);

Future<SassColor> visitColorExpression(ColorExpression node) async =>
node.value;

Expand Down
5 changes: 4 additions & 1 deletion lib/src/visitor/evaluate.dart
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_evaluate.dart.
// See tool/synchronize.dart for details.
//
// Checksum: f20e0967bae462f7d1728053fa0a41c09bcc0e03
// Checksum: ce258987d3496f06c82ca1f31df4a0ac778fe326

import 'async_evaluate.dart' show EvaluateResult;
export 'async_evaluate.dart' show EvaluateResult;
Expand Down Expand Up @@ -1242,6 +1242,9 @@ class _EvaluateVisitor
SassNumber visitNumberExpression(NumberExpression node) =>
new SassNumber(node.value, node.unit);

Value visitParenthesizedExpression(ParenthesizedExpression node) =>
node.expression.accept(this);

SassColor visitColorExpression(ColorExpression node) => node.value;

SassList visitListExpression(ListExpression node) => new SassList(
Expand Down
1 change: 1 addition & 0 deletions lib/src/visitor/interface/expression.dart
Expand Up @@ -17,6 +17,7 @@ abstract class ExpressionVisitor<T> {
T visitMapExpression(MapExpression node);
T visitNullExpression(NullExpression node);
T visitNumberExpression(NumberExpression node);
T visitParenthesizedExpression(ParenthesizedExpression node);
T visitSelectorExpression(SelectorExpression node);
T visitStringExpression(StringExpression node);
T visitUnaryOperationExpression(UnaryOperationExpression node);
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
@@ -1,5 +1,5 @@
name: sass
version: 1.14.3-dev
version: 1.14.3
description: A Sass implementation in Dart.
author: Dart Team <misc@dartlang.org>
homepage: https://github.com/sass/dart-sass
Expand Down
18 changes: 18 additions & 0 deletions test/cli/shared.dart
Expand Up @@ -253,6 +253,24 @@ void sharedTests(Future<TestProcess> runSass(Iterable<String> arguments)) {
await sass.shouldExit(65);
});

// Regression test for an issue mentioned in sass/linter#15
test(
"gracefully reports errors for binary operations with parentheized "
"operands", () async {
var sass = await runSass(["-"]);
sass.stdin.writeln("a {b: (#123) + (#456)}");
sass.stdin.close();
expect(
sass.stderr,
emitsInOrder([
'Error: Undefined operation "#123 + #456".',
"a {b: (#123) + (#456)}",
" ^^^^^^^^^^^^^^^",
" - 1:7 root stylesheet",
]));
await sass.shouldExit(65);
});

test("gracefully handles a non-partial next to a partial", () async {
await d.file("test.scss", "a {b: c}").create();
await d.file("_test.scss", "x {y: z}").create();
Expand Down

0 comments on commit a25bbb3

Please sign in to comment.