Merge pull request #96 from sass/extend-across-media

Forbid extending across media queries.

CHANGELOG.md

@@ -9,6 +9,8 @@
 
 * Disallow invalid function names.
 
+* Disallow extending across media queries.
+
 * Properly parse whitespace after `...` in argument declaration lists.
 
 * Support terse mixin syntax in the indented syntax.

README.md

@@ -137,6 +137,10 @@ Sass to update the reference behavior.
 9. Too many variable arguments passed to a function is an error. See
    [issue 1408][].
 
+10. Allow `@extend` to reach outside a media query if there's an identical
+    `@extend` defined outside that query. This isn't tracked explicitly, because
+    it'll be irrelevant when [issue 1050][] is fixed.
+
 [issue 1599]: https://github.com/sass/sass/issues/1599
 [issue 1126]: https://github.com/sass/sass/issues/1126
 [issue 2120]: https://github.com/sass/sass/issues/2120
@@ -146,5 +150,6 @@ Sass to update the reference behavior.
 [issue 1496]: https://github.com/sass/sass/issues/1496
 [issue 1525]: https://github.com/sass/sass/issues/1525
 [issue 1408]: https://github.com/sass/sass/issues/1408
+[issue 1050]: https://github.com/sass/sass/issues/1050
 
 Disclaimer: this is not an official Google product.

lib/src/ast/css/media_query.dart

@@ -3,6 +3,7 @@
 // https://opensource.org/licenses/MIT.
 
 import '../../parse/media_query.dart';
+import '../../utils.dart';
 
 /// A plain CSS media query, as used in `@media` and `@import`.
 class CssMediaQuery {
@@ -81,6 +82,14 @@ class CssMediaQuery {
         features: features.toList()..addAll(other.features));
   }
 
+  bool operator ==(other) =>
+      other is CssMediaQuery &&
+      other.modifier == modifier &&
+      other.type == type &&
+      listEquals(other.features, features);
+
+  int get hashCode => modifier.hashCode ^ type.hashCode ^ listHash(features);
+
   String toString() {
     var buffer = new StringBuffer();
     if (modifier != null) buffer.write("$modifier ");

lib/src/extend/extender.dart

@@ -25,6 +25,12 @@ class Extender {
   /// extensions.
   final _extensions = <SimpleSelector, Map<SelectorList, ExtendState>>{};
 
+  /// An expando from [CssStyleRule] to media query contexts.
+  ///
+  /// This tracks the contexts in which each style rule is defined. If a rule is
+  /// defined at the top level, it doesn't have an entry.
+  final _mediaContexts = new Expando<List<CssMediaQuery>>();
+
   /// An expando from [SimpleSelector]s to integers.
   ///
   /// This tracks the maximum specificity of the [ComplexSelector] that
@@ -50,35 +56,52 @@ class Extender {
   /// the stylesheet.
   static SelectorList extend(
           SelectorList selector, SelectorList source, SimpleSelector target) =>
-      new Extender()._extendList(selector, {
-        target: {source: new ExtendState.optional()}
-      });
+      new Extender()._extendList(
+          selector,
+          {
+            target: {source: new ExtendState.oneOff()}
+          },
+          null);
 
   /// Returns a copy of [selector] with [source] replaced by [target].
   static SelectorList replace(
           SelectorList selector, SelectorList source, SimpleSelector target) =>
       new Extender()._extendList(
           selector,
           {
-            target: {source: new ExtendState.optional()}
+            target: {source: new ExtendState.oneOff()}
           },
+          null,
           replace: true);
 
   /// Adds [selector] to this extender, associated with [span].
   ///
   /// Extends [selector] using any registered extensions, then returns an empty
   /// [CssStyleRule] with the resulting selector. If any more relevant
   /// extensions are added, the returned rule is automatically updated.
-  CssStyleRule addSelector(CssValue<SelectorList> selector, FileSpan span) {
+  ///
+  /// The [mediaContext] is the media query context in which the selector was
+  /// defined, or `null` if it was defined at the top level of the document.
+  CssStyleRule addSelector(CssValue<SelectorList> selector, FileSpan span,
+      [List<CssMediaQuery> mediaContext]) {
     for (var complex in selector.value.components) {
       _original[complex] = true;
     }
 
     if (_extensions.isNotEmpty) {
-      selector =
-          new CssValue(_extendList(selector.value, _extensions), selector.span);
+      try {
+        selector = new CssValue(
+            _extendList(selector.value, _extensions, mediaContext),
+            selector.span);
+      } on SassException catch (error) {
+        throw new SassException(
+            "From ${error.span.message('')}\n"
+            "${error.message}",
+            selector.span);
+      }
     }
     var rule = new CssStyleRule(selector, span);
+    if (mediaContext != null) _mediaContexts[rule] = mediaContext;
     _registerSelector(selector.value, rule);
 
     return rule;
@@ -107,21 +130,26 @@ class Extender {
   /// The [extender] is the selector for the style rule in which the extension
   /// is defined, and [target] is the selector passed to `@extend`. The [extend]
   /// provides the extend span and indicates whether the extension is optional.
+  ///
+  /// The [mediaContext] defines the media query context in which the extension
+  /// is defined. It can only extend selectors within the same context. A `null`
+  /// context indicates no media queries.
   void addExtension(
-      SelectorList extender, SimpleSelector target, ExtendRule extend) {
+      SelectorList extender, SimpleSelector target, ExtendRule extend,
+      [List<CssMediaQuery> mediaContext]) {
     var sources = _extensions.putIfAbsent(target, () => {});
     var existingState = sources[extender];
     if (existingState != null) {
       // If there's already an extend from [extender] to [target], we don't need
       // to re-run the extension. We may need to mark the extension as
       // mandatory, though.
-      if (!extend.isOptional) existingState.makeMandatory(extend.span);
+      existingState.addSource(extend.span, mediaContext,
+          optional: extend.isOptional);
       return;
     }
 
-    var state = extend.isOptional
-        ? new ExtendState.optional()
-        : new ExtendState.mandatory(extend.span);
+    var state =
+        new ExtendState(extend.span, mediaContext, optional: extend.isOptional);
     sources[extender] = state;
 
     for (var complex in extender.components) {
@@ -139,8 +167,17 @@ class Extender {
     var extensions = {
       target: {extender: state}
     };
-    for (var rule in rules.toList()) {
-      rule.selector.value = _extendList(rule.selector.value, extensions);
+    for (var rule in rules) {
+      try {
+        rule.selector.value =
+            _extendList(rule.selector.value, extensions, _mediaContexts[rule]);
+      } on SassException catch (error) {
+        throw new SassException(
+            "From ${rule.selector.span.message('')}\n"
+            "${error.message}",
+            error.span);
+      }
+
       _registerSelector(rule.selector.value, rule);
     }
   }
@@ -162,15 +199,18 @@ class Extender {
   /// Extends [list] using [extensions].
   ///
   /// If [replace] is `true`, this doesn't preserve the original selectors.
-  SelectorList _extendList(SelectorList list,
+  SelectorList _extendList(
+      SelectorList list,
       Map<SimpleSelector, Map<SelectorList, ExtendState>> extensions,
+      List<CssMediaQuery> mediaQueryContext,
       {bool replace: false}) {
     // This could be written more simply using [List.map], but we want to avoid
     // any allocations in the common case where no extends apply.
     List<List<ComplexSelector>> extended;
     for (var i = 0; i < list.components.length; i++) {
       var complex = list.components[i];
-      var result = _extendComplex(complex, extensions, replace: replace);
+      var result = _extendComplex(complex, extensions, mediaQueryContext,
+          replace: replace);
       if (result == null) {
         if (extended != null) extended.add([complex]);
       } else {
@@ -188,8 +228,10 @@ class Extender {
   /// [SelectorList].
   ///
   /// If [replace] is `true`, this doesn't preserve the original selectors.
-  List<List<ComplexSelector>> _extendComplex(ComplexSelector complex,
+  List<List<ComplexSelector>> _extendComplex(
+      ComplexSelector complex,
       Map<SimpleSelector, Map<SelectorList, ExtendState>> extensions,
+      List<CssMediaQuery> mediaQueryContext,
       {bool replace: false}) {
     // This could be written more simply using [List.map], but we want to avoid
     // any allocations in the common case where no extends apply.
@@ -198,7 +240,8 @@ class Extender {
     for (var i = 0; i < complex.components.length; i++) {
       var component = complex.components[i];
       if (component is CompoundSelector) {
-        var extended = _extendCompound(component, extensions, replace: replace);
+        var extended = _extendCompound(component, extensions, mediaQueryContext,
+            replace: replace);
         if (extended == null) {
           if (changed) {
             extendedNotExpanded.add([
@@ -253,8 +296,10 @@ class Extender {
   /// [SelectorList].
   ///
   /// If [replace] is `true`, this doesn't preserve the original selectors.
-  List<ComplexSelector> _extendCompound(CompoundSelector compound,
+  List<ComplexSelector> _extendCompound(
+      CompoundSelector compound,
       Map<SimpleSelector, Map<SelectorList, ExtendState>> extensions,
+      List<CssMediaQuery> mediaQueryContext,
       {bool replace: false}) {
     var original = compound;
 
@@ -263,7 +308,8 @@ class Extender {
       var simple = compound.components[i];
 
       var extendedPseudo = simple is PseudoSelector && simple.selector != null
-          ? _extendPseudo(simple, extensions, replace: replace)
+          ? _extendPseudo(simple, extensions, mediaQueryContext,
+              replace: replace)
           : null;
 
       if (extendedPseudo != null) {
@@ -292,6 +338,8 @@ class Extender {
               : unifyCompound(extenderBase.components, compoundWithoutSimple);
           if (unified == null) continue;
 
+          state.assertCompatibleMediaContext(mediaQueryContext);
+
           extended ??= replace
               ? []
               : [
@@ -334,10 +382,13 @@ class Extender {
   /// pseudo selectors.
   ///
   /// If [replace] is `true`, this doesn't preserve the original selectors.
-  List<PseudoSelector> _extendPseudo(PseudoSelector pseudo,
+  List<PseudoSelector> _extendPseudo(
+      PseudoSelector pseudo,
       Map<SimpleSelector, Map<SelectorList, ExtendState>> extensions,
+      List<CssMediaQuery> mediaQueryContext,
       {bool replace: false}) {
-    var extended = _extendList(pseudo.selector, extensions, replace: replace);
+    var extended = _extendList(pseudo.selector, extensions, mediaQueryContext,
+        replace: replace);
     if (identical(extended, pseudo.selector)) return null;
 
     // TODO: what do we do about placeholders in the selector? If we just

lib/src/extend/state.dart

@@ -4,38 +4,72 @@
 
 import 'package:source_span/source_span.dart';
 
+import '../ast/css.dart';
+import '../exception.dart';
+import '../utils.dart';
+
 /// The state of an extension for a given source and target.
 ///
 /// The source and target are represented externally, in the nested map that
 /// contains this state.
 class ExtendState {
   /// Whether this extension is optional.
-  bool get isOptional => _span == null;
+  bool get isOptional => _isOptional;
+  bool _isOptional;
 
   /// Whether this extension matched a selector.
   var isUsed = false;
 
-  /// The span for the `@extend` rule that should produce an error if this
-  /// extension doesn't match anything.
+  /// The media query context to which this extend is restricted, or `null` if
+  /// it can apply within any context.
+  List<CssMediaQuery> get mediaContext => _mediaContext;
+  List<CssMediaQuery> _mediaContext;
+
+  /// The span for an `@extend` rule that defined this extension.
   ///
-  /// This is `null` if and only if this extension is optional.
+  /// If any extend rule for this is extension is mandatory, this is guaranteed
+  /// to be a span for a mandatory rule.
   FileSpan get span => _span;
   FileSpan _span;
 
-  /// Creates a new optional extend state.
-  ExtendState.optional();
+  /// Creates a new extend state.
+  ExtendState(this._span, this._mediaContext, {bool optional: false})
+      : _isOptional = optional;
 
-  /// Creates a new mandatory extend state.
-  ///
-  /// The [span] is used in the error that's thrown if this extension doesn't
-  /// match anything.
-  ExtendState.mandatory(this._span);
+  /// Creates a one-off extend state that's not intended to be modified over time.
+  ExtendState.oneOff()
+      : _isOptional = true,
+        _mediaContext = null,
+        _span = null;
 
-  /// Marks this extension as mandatory.
-  ///
-  /// The [span] is used in the error that's thrown if this extension doesn't
-  /// match anything.
-  void makeMandatory(FileSpan span) {
+  /// Asserts that the [mediaContext] for a selector is compatible with the
+  /// query context for this extender.
+  void assertCompatibleMediaContext(List<CssMediaQuery> mediaContext) {
+    if (_mediaContext == null) return;
+    if (mediaContext != null && listEquals(_mediaContext, mediaContext)) return;
+
+    throw new SassException(
+        "You may not @extend selectors across media queries.", _span);
+  }
+
+  /// Indicates that the stylesheet contains another `@extend` with the same
+  /// source and target selectors, and the given [span] and [mediaContext].
+  void addSource(FileSpan span, List<CssMediaQuery> mediaContext,
+      {bool optional: false}) {
+    if (mediaContext != null) {
+      if (_mediaContext == null) {
+        _mediaContext = mediaContext;
+      } else if (!listEquals(_mediaContext, mediaContext)) {
+        throw new SassException(
+            "From ${_span.message('')}\n"
+            "You may not @extend the same selector from within different media "
+            "queries.",
+            span);
+      }
+    }
+
+    if (optional || !_isOptional) return;
     _span = span;
+    _isOptional = false;
   }
 }

lib/src/visitor/perform.dart

@@ -428,7 +428,7 @@ class _PerformVisitor
         targetText.span,
         () => new SimpleSelector.parse(targetText.value.trim(),
             allowParent: false));
-    _extender.addExtension(_selector.value, target, node);
+    _extender.addExtension(_selector.value, target, node, _mediaQueries);
     return null;
   }
 
@@ -770,7 +770,7 @@ class _PerformVisitor
     var selector =
         new CssValue<SelectorList>(parsedSelector, node.selector.span);
 
-    var rule = _extender.addSelector(selector, node.span);
+    var rule = _extender.addSelector(selector, node.span, _mediaQueries);
     _withParent(rule, () {
       _withSelector(rule.selector, () {
         for (var child in node.children) {