From f826ed2e543bb5685a901a2ac7c89daf0414afe3 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 13 Sep 2024 22:36:42 +0000 Subject: [PATCH] Stop emitting `mixed-decls` in a bunch of unnecessary cases (#2342) --- lib/src/ast/css/declaration.dart | 19 +++++++ lib/src/ast/css/modifiable/declaration.dart | 14 ++++- lib/src/ast/css/modifiable/node.dart | 1 - lib/src/ast/css/node.dart | 4 ++ lib/src/ast/css/stylesheet.dart | 1 + lib/src/async_compile.dart | 1 + lib/src/compile.dart | 3 +- lib/src/visitor/async_evaluate.dart | 53 +++++++++++++----- lib/src/visitor/evaluate.dart | 55 +++++++++++++------ lib/src/visitor/serialize.dart | 59 ++++++++++++++++++++- 10 files changed, 176 insertions(+), 34 deletions(-) diff --git a/lib/src/ast/css/declaration.dart b/lib/src/ast/css/declaration.dart index 4d5e906cd..2d8785dbf 100644 --- a/lib/src/ast/css/declaration.dart +++ b/lib/src/ast/css/declaration.dart @@ -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). @@ -16,6 +19,22 @@ abstract interface class CssDeclaration implements CssNode { /// The value of this declaration. CssValue 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 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 diff --git a/lib/src/ast/css/modifiable/declaration.dart b/lib/src/ast/css/modifiable/declaration.dart index 1acb292a7..3a0279307 100644 --- a/lib/src/ast/css/modifiable/declaration.dart +++ b/lib/src/ast/css/modifiable/declaration.dart @@ -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. @@ -16,6 +18,8 @@ final class ModifiableCssDeclaration extends ModifiableCssNode final CssValue name; final CssValue value; final bool parsedAsCustomProperty; + final List interleavedRules; + final Trace? trace; final FileSpan valueSpanForMap; final FileSpan span; @@ -23,8 +27,14 @@ final class ModifiableCssDeclaration extends ModifiableCssNode /// 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? interleavedRules, + this.trace, + FileSpan? valueSpanForMap}) + : interleavedRules = interleavedRules == null + ? const [] + : List.unmodifiable(interleavedRules), + valueSpanForMap = valueSpanForMap ?? value.span { if (parsedAsCustomProperty) { if (!isCustomProperty) { throw ArgumentError( diff --git a/lib/src/ast/css/modifiable/node.dart b/lib/src/ast/css/modifiable/node.dart index 1b27f8258..230cab2e7 100644 --- a/lib/src/ast/css/modifiable/node.dart +++ b/lib/src/ast/css/modifiable/node.dart @@ -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; diff --git a/lib/src/ast/css/node.dart b/lib/src/ast/css/node.dart index 29daba28d..04b6b839d 100644 --- a/lib/src/ast/css/node.dart +++ b/lib/src/ast/css/node.dart @@ -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; diff --git a/lib/src/ast/css/stylesheet.dart b/lib/src/ast/css/stylesheet.dart index 08a671cde..45d4d97bb 100644 --- a/lib/src/ast/css/stylesheet.dart +++ b/lib/src/ast/css/stylesheet.dart @@ -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 children; final FileSpan span; bool get isGroupEnd => false; diff --git a/lib/src/async_compile.dart b/lib/src/async_compile.dart index ea701f598..6598fcb57 100644 --- a/lib/src/async_compile.dart +++ b/lib/src/async_compile.dart @@ -173,6 +173,7 @@ Future _compileStylesheet( useSpaces: useSpaces, indentWidth: indentWidth, lineFeed: lineFeed, + logger: logger, sourceMap: sourceMap, charset: charset); diff --git a/lib/src/compile.dart b/lib/src/compile.dart index 6854dd68e..64f102566 100644 --- a/lib/src/compile.dart +++ b/lib/src/compile.dart @@ -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 @@ -182,6 +182,7 @@ CompileResult _compileStylesheet( useSpaces: useSpaces, indentWidth: indentWidth, lineFeed: lineFeed, + logger: logger, sourceMap: sourceMap, charset: charset); diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index eabadbc52..79b0f44db 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -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 = []; + 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); @@ -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('--')) { diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index 1aca4eee6..a555ac8cd 100644 --- a/lib/src/visitor/evaluate.dart +++ b/lib/src/visitor/evaluate.dart @@ -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 @@ -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 = []; + 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); @@ -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('--')) { diff --git a/lib/src/visitor/serialize.dart b/lib/src/visitor/serialize.dart index 4d036e6e3..a93c2fb8f 100644 --- a/lib/src/visitor/serialize.dart +++ b/lib/src/visitor/serialize.dart @@ -6,6 +6,7 @@ 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'; @@ -13,10 +14,13 @@ 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'; @@ -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; @@ -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(); @@ -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; @@ -138,6 +150,7 @@ final class _SerializeVisitor bool useSpaces = true, int? indentWidth, LineFeed? lineFeed, + Logger? logger, bool sourceMap = true}) : _buffer = sourceMap ? SourceMapBuffer() : NoSourceMapBuffer(), _style = style ?? OutputStyle.expanded, @@ -145,7 +158,8 @@ final class _SerializeVisitor _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"); } @@ -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); @@ -363,6 +404,22 @@ final class _SerializeVisitor } } + /// Returns the set of possible specificities which which [node] might match. + Set _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);