Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop emitting mixed-decls in a bunch of unnecessary cases #2342

Merged
merged 2 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions lib/src/ast/css/declaration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import 'package:meta/meta.dart';
import 'package:source_span/source_span.dart';
import 'package:stack_trace/stack_trace.dart';

import '../../value.dart';
import 'node.dart';
import 'style_rule.dart';
import 'value.dart';

/// A plain CSS declaration (that is, a `name: value` pair).
Expand All @@ -16,6 +19,22 @@ abstract interface class CssDeclaration implements CssNode {
/// The value of this declaration.
CssValue<Value> get value;

/// A list of style rules that appeared before this declaration in the Sass
/// input but after it in the CSS output.
///
/// These are used to emit mixed declaration deprecation warnings during
/// serialization, so we can check based on specificity whether the warnings
/// are really necessary without worrying about `@extend` potentially changing
/// things up.
@internal
List<CssStyleRule> get interleavedRules;

/// The stack trace indicating where this node was created.
///
/// This is used to emit interleaved declaration warnings, and is only set if
/// [interleavedRules] isn't empty.
Trace? get trace;

/// The span for [value] that should be emitted to the source map.
///
/// When the declaration's expression is just a variable, this is the span
Expand Down
14 changes: 12 additions & 2 deletions lib/src/ast/css/modifiable/declaration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
// https://opensource.org/licenses/MIT.

import 'package:source_span/source_span.dart';
import 'package:stack_trace/stack_trace.dart';

import '../../../value.dart';
import '../../../visitor/interface/modifiable_css.dart';
import '../declaration.dart';
import '../value.dart';
import '../style_rule.dart';
import 'node.dart';

/// A modifiable version of [CssDeclaration] for use in the evaluation step.
Expand All @@ -16,15 +18,23 @@ final class ModifiableCssDeclaration extends ModifiableCssNode
final CssValue<String> name;
final CssValue<Value> value;
final bool parsedAsCustomProperty;
final List<CssStyleRule> interleavedRules;
final Trace? trace;
final FileSpan valueSpanForMap;
final FileSpan span;

bool get isCustomProperty => name.value.startsWith('--');

/// Returns a new CSS declaration with the given properties.
ModifiableCssDeclaration(this.name, this.value, this.span,
{required this.parsedAsCustomProperty, FileSpan? valueSpanForMap})
: valueSpanForMap = valueSpanForMap ?? value.span {
{required this.parsedAsCustomProperty,
Iterable<CssStyleRule>? interleavedRules,
this.trace,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say this isn't set when interleavedRules is empty, but I could make an object that's the opposite of that. Should this have a validation that ensures trace will be null when interleavedRules is null or empty? Or should the documentation on the AST node say that the behavior of trace is undefined when interleavedRules is empty?

FileSpan? valueSpanForMap})
: interleavedRules = interleavedRules == null
? const []
: List.unmodifiable(interleavedRules),
valueSpanForMap = valueSpanForMap ?? value.span {
if (parsedAsCustomProperty) {
if (!isCustomProperty) {
throw ArgumentError(
Expand Down
1 change: 0 additions & 1 deletion lib/src/ast/css/modifiable/node.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import '../node.dart';
/// modification should only be done within the evaluation step, so the
/// unmodifiable types are used elsewhere to enforce that constraint.
abstract base class ModifiableCssNode extends CssNode {
/// The node that contains this, or `null` for the root [CssStylesheet] node.
ModifiableCssParentNode? get parent => _parent;
ModifiableCssParentNode? _parent;

Expand Down
4 changes: 4 additions & 0 deletions lib/src/ast/css/node.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ import 'style_rule.dart';
/// A statement in a plain CSS syntax tree.
@sealed
abstract class CssNode implements AstNode {
/// The node that contains this, or `null` for the root [CssStylesheet] node.
@internal
CssParentNode? get parent;

/// Whether this was generated from the last node in a nested Sass tree that
/// got flattened during evaluation.
bool get isGroupEnd;
Expand Down
1 change: 1 addition & 0 deletions lib/src/ast/css/stylesheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'node.dart';
///
/// This is the root plain CSS node. It contains top-level statements.
class CssStylesheet extends CssParentNode {
CssParentNode? get parent => null;
final List<CssNode> children;
final FileSpan span;
bool get isGroupEnd => false;
Expand Down
1 change: 1 addition & 0 deletions lib/src/async_compile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ Future<CompileResult> _compileStylesheet(
useSpaces: useSpaces,
indentWidth: indentWidth,
lineFeed: lineFeed,
logger: logger,
sourceMap: sourceMap,
charset: charset);

Expand Down
3 changes: 2 additions & 1 deletion lib/src/compile.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_compile.dart.
// See tool/grind/synchronize.dart for details.
//
// Checksum: 69b31749dc94c7f717e9d395327e4209c4d3feb0
// Checksum: 4d72aeb3c39a2e607d1889755e07b7e489eddfa6
//
// ignore_for_file: unused_import

Expand Down Expand Up @@ -182,6 +182,7 @@ CompileResult _compileStylesheet(
useSpaces: useSpaces,
indentWidth: indentWidth,
lineFeed: lineFeed,
logger: logger,
sourceMap: sourceMap,
charset: charset);

Expand Down
53 changes: 39 additions & 14 deletions lib/src/visitor/async_evaluate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1198,20 +1198,43 @@ final class _EvaluateVisitor
node.span);
}

if (_parent.parent!.children.last case var sibling
when _parent != sibling) {
_warn(
"Sass's behavior for declarations that appear after nested\n"
"rules will be changing to match the behavior specified by CSS in an "
"upcoming\n"
"version. To keep the existing behavior, move the declaration above "
"the nested\n"
"rule. To opt into the new behavior, wrap the declaration in `& "
"{}`.\n"
"\n"
"More info: https://sass-lang.com/d/mixed-decls",
MultiSpan(node.span, 'declaration', {sibling.span: 'nested rule'}),
Deprecation.mixedDecls);
var siblings = _parent.parent!.children;
var interleavedRules = <CssStyleRule>[];
if (siblings.last != _parent &&
// Reproduce this condition from [_warn] so that we don't add anything to
// [interleavedRules] for declarations in dependencies.
!(_quietDeps &&
(_inDependency || (_currentCallable?.inDependency ?? false)))) {
loop:
for (var sibling in siblings.skip(siblings.indexOf(_parent) + 1)) {
switch (sibling) {
case CssComment():
continue loop;

case CssStyleRule rule:
interleavedRules.add(rule);

case _:
// Always warn for siblings that aren't style rules, because they
// add no specificity and they're nested in the same parent as this
// declaration.
_warn(
"Sass's behavior for declarations that appear after nested\n"
"rules will be changing to match the behavior specified by CSS "
"in an upcoming\n"
"version. To keep the existing behavior, move the declaration "
"above the nested\n"
"rule. To opt into the new behavior, wrap the declaration in "
"`& {}`.\n"
"\n"
"More info: https://sass-lang.com/d/mixed-decls",
MultiSpan(
node.span, 'declaration', {sibling.span: 'nested rule'}),
Deprecation.mixedDecls);
interleavedRules.clear();
break;
}
}
}

var name = await _interpolationToValue(node.name, warnForColor: true);
Expand All @@ -1227,6 +1250,8 @@ final class _EvaluateVisitor
_parent.addChild(ModifiableCssDeclaration(
name, CssValue(value, expression.span), node.span,
parsedAsCustomProperty: node.isCustomProperty,
interleavedRules: interleavedRules,
trace: interleavedRules.isEmpty ? null : _stackTrace(node.span),
valueSpanForMap:
_sourceMap ? node.value.andThen(_expressionNode)?.span : null));
} else if (name.value.startsWith('--')) {
Expand Down
55 changes: 40 additions & 15 deletions lib/src/visitor/evaluate.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_evaluate.dart.
// See tool/grind/synchronize.dart for details.
//
// Checksum: de25b9055a73f1c7ebe7a707139e6c789a2866dd
// Checksum: ca67afd2df1c970eb887d4a24c7fe838c2aaec60
//
// ignore_for_file: unused_import

Expand Down Expand Up @@ -1196,20 +1196,43 @@ final class _EvaluateVisitor
node.span);
}

if (_parent.parent!.children.last case var sibling
when _parent != sibling) {
_warn(
"Sass's behavior for declarations that appear after nested\n"
"rules will be changing to match the behavior specified by CSS in an "
"upcoming\n"
"version. To keep the existing behavior, move the declaration above "
"the nested\n"
"rule. To opt into the new behavior, wrap the declaration in `& "
"{}`.\n"
"\n"
"More info: https://sass-lang.com/d/mixed-decls",
MultiSpan(node.span, 'declaration', {sibling.span: 'nested rule'}),
Deprecation.mixedDecls);
var siblings = _parent.parent!.children;
var interleavedRules = <CssStyleRule>[];
if (siblings.last != _parent &&
// Reproduce this condition from [_warn] so that we don't add anything to
// [interleavedRules] for declarations in dependencies.
!(_quietDeps &&
(_inDependency || (_currentCallable?.inDependency ?? false)))) {
loop:
for (var sibling in siblings.skip(siblings.indexOf(_parent) + 1)) {
switch (sibling) {
case CssComment():
continue loop;

case CssStyleRule rule:
interleavedRules.add(rule);

case _:
// Always warn for siblings that aren't style rules, because they
// add no specificity and they're nested in the same parent as this
// declaration.
_warn(
"Sass's behavior for declarations that appear after nested\n"
"rules will be changing to match the behavior specified by CSS "
"in an upcoming\n"
"version. To keep the existing behavior, move the declaration "
"above the nested\n"
"rule. To opt into the new behavior, wrap the declaration in "
"`& {}`.\n"
"\n"
"More info: https://sass-lang.com/d/mixed-decls",
MultiSpan(
node.span, 'declaration', {sibling.span: 'nested rule'}),
Deprecation.mixedDecls);
interleavedRules.clear();
break;
}
}
}

var name = _interpolationToValue(node.name, warnForColor: true);
Expand All @@ -1225,6 +1248,8 @@ final class _EvaluateVisitor
_parent.addChild(ModifiableCssDeclaration(
name, CssValue(value, expression.span), node.span,
parsedAsCustomProperty: node.isCustomProperty,
interleavedRules: interleavedRules,
trace: interleavedRules.isEmpty ? null : _stackTrace(node.span),
valueSpanForMap:
_sourceMap ? node.value.andThen(_expressionNode)?.span : null));
} else if (name.value.startsWith('--')) {
Expand Down
59 changes: 58 additions & 1 deletion lib/src/visitor/serialize.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,21 @@ import 'dart:math' as math;
import 'dart:typed_data';

import 'package:charcode/charcode.dart';
import 'package:collection/collection.dart';
import 'package:source_maps/source_maps.dart';
import 'package:string_scanner/string_scanner.dart';

import '../ast/css.dart';
import '../ast/node.dart';
import '../ast/selector.dart';
import '../color_names.dart';
import '../deprecation.dart';
import '../exception.dart';
import '../logger.dart';
import '../parse/parser.dart';
import '../utils.dart';
import '../util/character.dart';
import '../util/multi_span.dart';
import '../util/no_source_map_buffer.dart';
import '../util/nullable.dart';
import '../util/number.dart';
Expand Down Expand Up @@ -48,6 +52,7 @@ SerializeResult serialize(CssNode node,
bool useSpaces = true,
int? indentWidth,
LineFeed? lineFeed,
Logger? logger,
bool sourceMap = false,
bool charset = true}) {
indentWidth ??= 2;
Expand All @@ -57,6 +62,7 @@ SerializeResult serialize(CssNode node,
useSpaces: useSpaces,
indentWidth: indentWidth,
lineFeed: lineFeed,
logger: logger,
sourceMap: sourceMap);
node.accept(visitor);
var css = visitor._buffer.toString();
Expand Down Expand Up @@ -128,6 +134,12 @@ final class _SerializeVisitor
/// The characters to use for a line feed.
final LineFeed _lineFeed;

/// The logger to use to print warnings.
///
/// This should only be used for statement-level serialization. It's not
/// guaranteed to be the main user-provided logger for expressions.
final Logger _logger;

/// Whether we're emitting compressed output.
bool get _isCompressed => _style == OutputStyle.compressed;

Expand All @@ -138,14 +150,16 @@ final class _SerializeVisitor
bool useSpaces = true,
int? indentWidth,
LineFeed? lineFeed,
Logger? logger,
bool sourceMap = true})
: _buffer = sourceMap ? SourceMapBuffer() : NoSourceMapBuffer(),
_style = style ?? OutputStyle.expanded,
_inspect = inspect,
_quote = quote,
_indentCharacter = useSpaces ? $space : $tab,
_indentWidth = indentWidth ?? 2,
_lineFeed = lineFeed ?? LineFeed.lf {
_lineFeed = lineFeed ?? LineFeed.lf,
_logger = logger ?? const Logger.stderr() {
RangeError.checkValueInInterval(_indentWidth, 0, 10, "indentWidth");
}

Expand Down Expand Up @@ -329,6 +343,33 @@ final class _SerializeVisitor
}

void visitCssDeclaration(CssDeclaration node) {
if (node.interleavedRules.isNotEmpty) {
var declSpecificities = _specificities(node.parent!);
for (var rule in node.interleavedRules) {
var ruleSpecificities = _specificities(rule);

// If the declaration can never match with the same specificity as one
// of its sibling rules, then ordering will never matter and there's no
// need to warn about the declaration being re-ordered.
if (!declSpecificities.any(ruleSpecificities.contains)) continue;

_logger.warnForDeprecation(
Deprecation.mixedDecls,
"Sass's behavior for declarations that appear after nested\n"
"rules will be changing to match the behavior specified by CSS in an "
"upcoming\n"
"version. To keep the existing behavior, move the declaration above "
"the nested\n"
"rule. To opt into the new behavior, wrap the declaration in `& "
"{}`.\n"
"\n"
"More info: https://sass-lang.com/d/mixed-decls",
span:
MultiSpan(node.span, 'declaration', {rule.span: 'nested rule'}),
trace: node.trace);
}
}

_writeIndentation();

_write(node.name);
Expand Down Expand Up @@ -363,6 +404,22 @@ final class _SerializeVisitor
}
}

/// Returns the set of possible specificities which which [node] might match.
Set<int> _specificities(CssParentNode node) {
if (node case CssStyleRule rule) {
// Plain CSS style rule nesting implicitly wraps parent selectors in
// `:is()`, so they all match with the highest specificity among any of
// them.
var parent = node.parent.andThen(_specificities)?.max ?? 0;
return {
for (var selector in rule.selector.components)
parent + selector.specificity
};
} else {
return node.parent.andThen(_specificities) ?? const {0};
}
}

/// Emits the value of [node], with all newlines followed by whitespace
void _writeFoldedValue(CssDeclaration node) {
var scanner = StringScanner((node.value.value as SassString).text);
Expand Down