From 829835e9d21c74bc1418f78ea009b448df083608 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 21 May 2021 13:40:07 -0700 Subject: [PATCH] Only emit each warning once per source location --- CHANGELOG.md | 4 ++++ lib/src/visitor/async_evaluate.dart | 30 +++++++++++++++++++-------- lib/src/visitor/evaluate.dart | 32 ++++++++++++++++++++--------- test/dart_api/logger_test.dart | 28 +++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60aab93c5..104c83a8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 1.33.1 + +* Don't emit the same warning in the same location multiple times. + ## 1.33.0 * Deprecate the use of `/` for division. The new `math.div()` function should be diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 4f60ef35f..5f4a42c37 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -148,6 +148,13 @@ class _EvaluateVisitor /// The logger to use to print warnings. final Logger _logger; + /// A set of message/location pairs for warnings that have been emitted via + /// [_warn]. + /// + /// We only want to emit one warning per location, to avoid blowing up users' + /// consoles with redundant warnings. + final _warningsEmitted = >{}; + /// Whether to track source map information. final bool _sourceMap; @@ -1936,7 +1943,7 @@ class _EvaluateVisitor } if (node.isGlobal && !_environment.globalVariableExists(node.name)) { - _logger.warn( + _warn( _environment.atRoot ? "As of Dart Sass 2.0.0, !global assignments won't be able to\n" "declare new variables. Since this assignment is at the root " @@ -1946,8 +1953,7 @@ class _EvaluateVisitor "declare new variables. Consider adding " "`${node.originalName}: null` at the root of the\n" "stylesheet.", - span: node.span, - trace: _stackTrace(node.span), + node.span, deprecation: true); } @@ -1987,9 +1993,13 @@ class _EvaluateVisitor Future visitWarnRule(WarnRule node) async { var value = await _addExceptionSpanAsync(node, () => node.expression.accept(this)); - _logger.warn( - value is SassString ? value.text : _serialize(value, node.expression), - trace: _stackTrace(node.span)); + var message = + value is SassString ? value.text : _serialize(value, node.expression); + + // Don't use [_warn] because we don't want to pass the span to the logger. + if (_warningsEmitted.add(Tuple2(message, node.span))) { + _warn(message, node.span); + } return null; } @@ -3087,9 +3097,11 @@ class _EvaluateVisitor } /// Emits a warning with the given [message] about the given [span]. - void _warn(String message, FileSpan span, {bool deprecation = false}) => - _logger.warn(message, - span: span, trace: _stackTrace(span), deprecation: deprecation); + void _warn(String message, FileSpan span, {bool deprecation = false}) { + if (!_warningsEmitted.add(Tuple2(message, span))) return; + _logger.warn(message, + span: span, trace: _stackTrace(span), deprecation: deprecation); + } /// Returns a [SassRuntimeException] with the given [message]. /// diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index c478d374f..772ec9923 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: 1fc6b9e6018eba3ac520464abb56e686f4cb9886 +// Checksum: 75235f66937f2acdfb9d433bd25f821c63d5ff53 // // ignore_for_file: unused_import @@ -156,6 +156,13 @@ class _EvaluateVisitor /// The logger to use to print warnings. final Logger _logger; + /// A set of message/location pairs for warnings that have been emitted via + /// [_warn]. + /// + /// We only want to emit one warning per location, to avoid blowing up users' + /// consoles with redundant warnings. + final _warningsEmitted = >{}; + /// Whether to track source map information. final bool _sourceMap; @@ -1928,7 +1935,7 @@ class _EvaluateVisitor } if (node.isGlobal && !_environment.globalVariableExists(node.name)) { - _logger.warn( + _warn( _environment.atRoot ? "As of Dart Sass 2.0.0, !global assignments won't be able to\n" "declare new variables. Since this assignment is at the root " @@ -1938,8 +1945,7 @@ class _EvaluateVisitor "declare new variables. Consider adding " "`${node.originalName}: null` at the root of the\n" "stylesheet.", - span: node.span, - trace: _stackTrace(node.span), + node.span, deprecation: true); } @@ -1977,9 +1983,13 @@ class _EvaluateVisitor Value? visitWarnRule(WarnRule node) { var value = _addExceptionSpan(node, () => node.expression.accept(this)); - _logger.warn( - value is SassString ? value.text : _serialize(value, node.expression), - trace: _stackTrace(node.span)); + var message = + value is SassString ? value.text : _serialize(value, node.expression); + + // Don't use [_warn] because we don't want to pass the span to the logger. + if (_warningsEmitted.add(Tuple2(message, node.span))) { + _warn(message, node.span); + } return null; } @@ -3058,9 +3068,11 @@ class _EvaluateVisitor } /// Emits a warning with the given [message] about the given [span]. - void _warn(String message, FileSpan span, {bool deprecation = false}) => - _logger.warn(message, - span: span, trace: _stackTrace(span), deprecation: deprecation); + void _warn(String message, FileSpan span, {bool deprecation = false}) { + if (!_warningsEmitted.add(Tuple2(message, span))) return; + _logger.warn(message, + span: span, trace: _stackTrace(span), deprecation: deprecation); + } /// Returns a [SassRuntimeException] with the given [message]. /// diff --git a/test/dart_api/logger_test.dart b/test/dart_api/logger_test.dart index b22564ff9..78140e6eb 100644 --- a/test/dart_api/logger_test.dart +++ b/test/dart_api/logger_test.dart @@ -4,6 +4,8 @@ @TestOn('vm') +import 'dart:async'; + import 'package:test/test.dart'; import 'package:source_span/source_span.dart'; import 'package:stack_trace/stack_trace.dart'; @@ -29,6 +31,32 @@ void main() { })); }); + test("passes multiple messages with different locations to the logger", () { + var controller = StreamController(); + compileString(''' + @mixin foo {@warn heck} + @include foo; + @warn heck; + ''', + logger: _TestLogger.withWarn((message, + {span, trace, deprecation = false}) => + controller.add(message))); + + expect(controller.stream, emitsInOrder(["heck", "heck"])); + }); + + test("only passes a message with a single location to the logger once", () { + var count = 0; + compileString(r''' + @for $i from 1 to 5 { + @warn heck; + } + ''', + logger: _TestLogger.withWarn( + (message, {span, trace, deprecation = false}) => count++)); + expect(count, equals(1)); + }); + test("stringifies the argument", () { var mustBeCalled = expectAsync0(() {}); compileString('@warn #abc', logger: