Index: pkg/csslib/lib/src/analyzer.dart |
diff --git a/pkg/csslib/lib/src/analyzer.dart b/pkg/csslib/lib/src/analyzer.dart |
index 9e366a5fdfcc1b4d634dfc94512d5e809a9b5989..c16d9f07c551fbbb993be7aa08107e9794c1be41 100644 |
--- a/pkg/csslib/lib/src/analyzer.dart |
+++ b/pkg/csslib/lib/src/analyzer.dart |
@@ -23,16 +23,27 @@ part of csslib.parser; |
class Analyzer { |
final List<StyleSheet> _styleSheets; |
final Messages _messages; |
+ MixinDefinitions mixinDefs; |
VarDefinitions varDefs; |
Analyzer(this._styleSheets, this._messages); |
void run() { |
+ mixinDefs = new MixinDefinitions(_styleSheets); |
nweiz
2013/09/18 22:40:54
It's not correct to locate all mixin definitions b
terry
2013/10/09 03:40:33
Yeah, I liked the idea of hoisting like a class bu
|
+ |
+ // Any mixin cycles? |
+ var mixinCycles = findAllMixinCycles(); |
+ for (var cycle in mixinCycles) { |
+ _messages.warning("mixin cycle detected @mixin ${cycle.def.name}", |
nweiz
2013/09/18 22:40:54
"Mixin cycle detected for @mixin ${cycle.def.name}
terry
2013/10/09 03:40:33
Without hoisting cycle detection is removed.
On 20
|
+ cycle.def.span); |
+ // TODO(terry): What if no mixin definition for a @include an error? |
nweiz
2013/09/18 22:40:54
"What if there are no mixin definitions for an @in
|
+ } |
+ |
varDefs = new VarDefinitions(_styleSheets); |
- // Any cycles? |
- var cycles = findAllCycles(); |
- for (var cycle in cycles) { |
+ // Any var cycles? |
+ var varCycles = findAllVarCycles(); |
nweiz
2013/09/18 22:40:54
Variables should be eagerly evaluated, so cycle de
terry
2013/10/09 03:40:33
Yes also removed variable hoisting.
On 2013/09/18
|
+ for (var cycle in varCycles) { |
_messages.warning("var cycle detected var-${cycle.definedName}", |
cycle.span); |
// TODO(terry): What if no var definition for a var usage an error? |
@@ -42,7 +53,28 @@ class Analyzer { |
// Remove any var definition from the stylesheet that has a cycle. |
_styleSheets.forEach((styleSheet) => |
- new RemoveVarDefinitions(cycles).visitStyleSheet(styleSheet)); |
+ new RemoveVarDefinitions(varCycles).visitStyleSheet(styleSheet)); |
+ |
+ // Expand top-level @include. |
+ _styleSheets.forEach((styleSheet) => |
+ new ExpandTopLevelIncludes(mixinDefs, _messages) |
+ ..visitStyleSheet(styleSheet)); |
nweiz
2013/09/18 22:40:54
Running separate visitors for all of these seems i
terry
2013/10/09 03:40:33
Yes, for now that is the mode we've taken in the d
|
+ |
+ // Expand @include in declarations. |
nweiz
2013/09/18 22:40:54
Nit: extra space before "@include".
terry
2013/10/09 03:40:33
Done.
|
+ _styleSheets.forEach((styleSheet) => |
+ new ExpandDeclarationIncludes(styleSheet, mixinDefs, _messages) |
+ ..visitStyleSheet(styleSheet)); |
+ |
+ // Any missing @mixins only check if everything is good up to this point. |
nweiz
2013/09/18 22:40:54
I don't understand what this comment means.
terry
2013/10/09 03:40:33
Functionality removed when hoisting of mixin remov
|
+ if (_messages.messages.isEmpty) { |
+ _styleSheets.forEach((styleSheet) => |
+ new IncludesMissingMixins(mixinDefs, this._messages) |
+ ..visitStyleSheet(styleSheet)); |
+ } |
+ |
+ // Remove all @mixin and @include |
+ _styleSheets.forEach((styleSheet) => |
+ new RemoveMixinsAndIncludes()..visitStyleSheet(styleSheet)); |
nweiz
2013/09/18 22:40:54
Style nit: indent +2 spaces.
terry
2013/10/09 03:40:33
Code removed.
On 2013/09/18 22:40:54, nweiz wrote:
|
// Expand any nested selectors using selector desendant combinator to |
// signal CSS inheritance notation. |
@@ -51,7 +83,83 @@ class Analyzer { |
..flatten(styleSheet)); |
} |
- List<VarDefinition> findAllCycles() { |
+ List<MixinDefinition> findAllMixinCycles() { |
+ var cycles = []; |
+ |
+ mixinDefs.map.values.forEach((value) { |
nweiz
2013/09/18 22:40:54
As John's mentioned elsewhere, use a for loop rath
terry
2013/10/09 03:40:33
Cycle code removed.
On 2013/09/18 22:40:54, nweiz
|
+ if (hasMixinCycle(value.includes)) cycles.add(value); |
+ }); |
+ |
+ // Update our local list of known mixinDefinitions remove any mixins with a |
+ // cycle. So the same mixin cycle isn't reported for each style sheet |
+ // processed. |
+ for (var cycle in cycles) { |
+ mixinDefs.map.remove(cycle.def.name); |
+ } |
+ |
+ return cycles; |
+ } |
+ |
+ /** |
+ * [includes] list of IncludeDirective or IncludeMixinAtDeclaration. |
nweiz
2013/09/18 22:40:54
If you're going to include a doc comment, it shoul
terry
2013/10/09 03:40:33
Cycle code removed.
On 2013/09/18 22:40:54, nweiz
|
+ */ |
+ bool hasMixinCycle(List includes, {Set<String> visiting, |
+ Set<String> visited}) { |
+ if (visiting == null) visiting = new Set(); |
+ if (visited == null) visited = new Set(); |
nweiz
2013/09/18 22:40:54
Explain the function of [visited] and how it diffe
terry
2013/10/09 03:40:33
cycle code removed.
On 2013/09/18 22:40:54, nweiz
|
+ |
+ bool cycleDetected = false; |
+ |
+ var name; |
+ for (var include in includes) { |
+ if (include is IncludeMixinAtDeclaration) { |
+ // Get to the IncludeDirective. |
+ name = include.include.name; |
+ } else { |
+ name = include.name; |
+ } |
+ |
+ if (visiting.contains(name)) { |
+ cycleDetected = true; |
+ break; |
+ } |
+ if (!visited.contains(name)) { |
+ visiting.add(name); |
+ visited.add(name); |
+ } |
+ if (mixinDefs.map[name] != null) { |
+ if (hasMixinCycle(mixinDefs.map[name].includes, visiting: visiting, |
+ visited: visited)) { |
+ cycleDetected = true; |
+ break; |
+ } |
+ } |
+ |
+ |
+ |
+/* |
Jennifer Messerly
2013/09/18 06:44:15
remove?
terry
2013/10/09 03:40:33
entire function removed.
On 2013/09/18 06:44:15, J
|
+ for (var usage in mixinInclude(mixinDefs.map[name].includes)) { |
+ if (hasMixinCycle(usage.name, visiting: visiting, visited: visited)) { |
+ cycleDetected = true; |
+ break; |
+ } |
+ } |
+*/ |
+ visiting.remove(include); |
+ } |
+ |
+ return cycleDetected; |
+ } |
+ |
+ // TODO(terry): Only checks @includes in declarations not ruleset top-level. |
+ Iterable<IncludeDirective> mixinInclude(List includes) => |
+ includes.where((e) => e is IncludeMixinAtDeclaration); |
+ |
+ // ===================== |
+ // Var cycles functions: |
+ // ===================== |
+ |
+ List<VarDefinition> findAllVarCycles() { |
var cycles = []; |
varDefs.map.values.forEach((value) { |
@@ -511,3 +619,253 @@ class _MediaRulesReplacer extends Visitor { |
} |
} |
} |
+ |
+class MixinDef { |
+ MixinDefinition def; |
+ List<IncludeDirective> includes; |
+ |
+ MixinDef(this.def, this.includes); |
+} |
+ |
+/** Find all mixin definitions from a list of stylesheets. */ |
+class MixinDefinitions extends Visitor { |
+ /** Map of variable name key to it's definition. */ |
+ final Map<String, MixinDef> map = new Map<String, MixinDef>(); |
nweiz
2013/09/18 22:40:54
This type declaration is unnecessary. The type is
terry
2013/10/09 03:40:33
Yep but editor code completion didn't work w/o it
|
+ |
+ var _includes = []; |
+ |
+ MixinDefinitions(List<StyleSheet> styleSheets) { |
+ for (var styleSheet in styleSheets) { |
+ visitTree(styleSheet); |
+ } |
+ } |
+ |
+ void visitIncludeDirective(IncludeDirective node) { |
+ _includes.add(node); |
+ } |
+ |
+ void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) { |
+ _includes.add(node); |
+ } |
+ |
+ void visitMixinRulesetDirective(MixinRulesetDirective node) { |
+ _includes = []; |
+ |
+ super.visitMixinRulesetDirective(node); |
+ |
+ // Replace with latest top-level mixin definition. |
+ map[node.name] = new MixinDef(node, _includes); |
+ _includes = []; |
+ } |
+ |
+ void visitMixinDeclarationDirective(MixinDeclarationDirective node) { |
+ _includes = []; |
+ |
+ super.visitMixinDeclarationDirective(node); |
+ |
+ // Replace with latest mixin definition. |
+ map[node.name] = new MixinDef(node, _includes); |
+ _includes = []; |
+ } |
+} |
+ |
+/** |
+ * Expand all @include at the top-level the ruleset(s) associated with the |
+ * mixin. |
+ */ |
+class ExpandTopLevelIncludes extends Visitor { |
+ StyleSheet _styleSheet; |
+ final MixinDefinitions defs; |
+ final Messages _messages; |
+ |
+ ExpandTopLevelIncludes(this.defs, this._messages); |
+ |
+ void visitStyleSheet(StyleSheet ss) { |
+ _styleSheet = ss; |
+ super.visitStyleSheet(ss); |
+ _styleSheet = null; |
+ } |
+ |
+ void visitIncludeDirective(IncludeDirective node) { |
+ if (defs.map.containsKey(node.name)) { |
+ var mixinDef = defs.map[node.name].def; |
+ if (mixinDef is MixinRulesetDirective) { |
+ _TopLevelIncludeReplacer.replace(_styleSheet, node, mixinDef.rulesets); |
+ } |
+ } |
+ super.visitIncludeDirective(node); |
+ } |
+} |
+ |
+/** @include as a top-level with ruleset(s). */ |
+class _TopLevelIncludeReplacer extends Visitor { |
+ final IncludeDirective _include; |
+ final List<RuleSet> _newRules; |
+ bool _foundAndReplaced = false; |
+ |
+ /** |
+ * Look for the [ruleSet] inside of an @media directive; if found then replace |
+ * with the [newRules]. If [ruleSet] is found and replaced return true. |
+ */ |
+ static bool replace(StyleSheet styleSheet, IncludeDirective include, |
nweiz
2013/09/18 22:40:54
You have three different ways of invoking a visito
terry
2013/10/09 03:40:33
Function was re-written this no longer exist.
On 2
|
+ List<RuleSet>newRules) { |
+ var visitor = new _TopLevelIncludeReplacer(include, newRules); |
+ visitor.visitStyleSheet(styleSheet); |
+ return visitor._foundAndReplaced; |
nweiz
2013/09/18 22:40:54
This value is never used.
terry
2013/10/09 03:40:33
function re-written not an issue now.
On 2013/09/1
|
+ } |
+ |
+ _TopLevelIncludeReplacer(this._include, this._newRules); |
+ |
+ visitStyleSheet(StyleSheet node) { |
+ var index = node.topLevels.indexOf(_include); |
+ if (index != -1) { |
+ node.topLevels.insertAll(index + 1, _newRules); |
+ node.topLevels.replaceRange(index, index + 1, [new NoOp()]); |
+ _foundAndReplaced = true; |
+ } |
+ super.visitStyleSheet(node); |
+ } |
+ |
+ void visitMixinRulesetDirective(MixinRulesetDirective node) { |
+ var index = node.rulesets.indexOf(_include as dynamic); |
nweiz
2013/09/18 22:40:54
What is "as dynamic" doing here?
terry
2013/10/09 03:40:33
_include can be different either declarative vs/ r
|
+ if (index != -1) { |
+ node.rulesets.insertAll(index + 1, _newRules); |
+ node.rulesets.replaceRange(index, index + 1, [new NoOp()]); |
nweiz
2013/09/18 22:40:54
Why aren't you replacing the include with _newRule
terry
2013/10/09 03:40:33
Only the resolve the @include once.
On 2013/09/18
|
+ _foundAndReplaced = true; |
+ } |
+ super.visitMixinRulesetDirective(node); |
+ } |
+} |
+ |
+/** Expand all @include inside of a declaration associated with a mixin. */ |
+class ExpandDeclarationIncludes extends Visitor { |
+ final StyleSheet _styleSheet; |
+ final MixinDefinitions defs; |
+ final Messages _messages; |
+ |
+ ExpandDeclarationIncludes(this._styleSheet, this.defs, this._messages); |
+ |
+ bool _allIncludes(rulesets) => |
+ rulesets.every((rule) => rule is IncludeDirective || rule is NoOp); |
+ |
+ void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) { |
+ if (defs.map.containsKey(node.include.name)) { |
nweiz
2013/09/18 22:40:54
Short-circuit if this is false to avoid extra nest
terry
2013/10/09 03:40:33
Really, seems clunky to call super for the short-c
|
+ var mixinDef = defs.map[node.include.name].def; |
+ if (mixinDef is MixinDeclarationDirective) { |
+ _IncludeReplacer.replace(_styleSheet, node, |
+ mixinDef.declarations.declarations); |
+ } else if (mixinDef is MixinRulesetDirective) { |
+ // We're a list of @include(s) inside of a mixin ruleset if so then |
+ // we'll convert to a list of IncludeMixinAtDeclaration(s). |
nweiz
2013/09/18 22:40:54
It doesn't make sense to say "We're X. If so, ..."
terry
2013/10/09 03:40:33
yep, changed.
On 2013/09/18 22:40:54, nweiz wrote:
|
+ var orgRulesets = mixinDef.rulesets; |
nweiz
2013/09/18 22:40:54
What does "org" mean?
terry
2013/10/09 03:40:33
original - missing 'i' origRulesets
On 2013/09/18
|
+ var rulesets = []; |
+ if (orgRulesets.every((ruleset) => ruleset is IncludeDirective)) { |
+ orgRulesets.forEach((ruleset) { |
+ rulesets.add(new IncludeMixinAtDeclaration(ruleset, ruleset.span)); |
+ }); |
+ _IncludeReplacer.replace(_styleSheet, node, rulesets); |
+ } else { |
+ _messages.warning( |
+ "Trying to use top-level ruleset in a declaration - " |
+ "@include ${node.include.name}", |
+ node.span); |
+ } |
+ } |
+ } |
+ |
+ super.visitIncludeMixinAtDeclaration(node); |
+ } |
+} |
+ |
+/** @include as a top-level with ruleset(s). */ |
+class _IncludeReplacer extends Visitor { |
+ final _include; |
+ final List<Declaration> _newDeclarations; |
+ bool _foundAndReplaced = false; |
+ |
+ /** |
+ * Look for the [ruleSet] inside of an @media directive; if found then replace |
nweiz
2013/09/18 22:40:54
"a @media"
terry
2013/10/09 03:40:33
Done.
|
+ * with the [newRules]. If [ruleSet] is found and replaced return true. |
+ */ |
+ static bool replace(StyleSheet ss, var include, |
+ List<Declaration> newDeclarations) { |
+ var visitor = new _IncludeReplacer(include, newDeclarations); |
+ visitor.visitStyleSheet(ss); |
+ return visitor._foundAndReplaced; |
nweiz
2013/09/18 22:40:54
This is also never used.
terry
2013/10/09 03:40:33
Done.
|
+ } |
+ |
+ _IncludeReplacer(this._include, this._newDeclarations); |
+ |
+ void visitDeclarationGroup(DeclarationGroup node) { |
+ var index = -1; |
+ var i = 0; |
+ for (var decl in node.declarations) { |
+ if (decl == _include) { |
+ index = i; |
+ break; |
+ } else if (decl is IncludeMixinAtDeclaration) { |
+ if (decl.include == _include) { |
nweiz
2013/09/18 22:40:54
Use "&&" rather than nested if statements.
terry
2013/10/09 03:40:33
While re-writing not an issue anymore.
On 2013/09/
|
+ index = i; |
+ break; |
+ } |
+ } |
+ i++; |
+ } |
nweiz
2013/09/18 22:40:54
This i++/for/break stuff is really complicated and
terry
2013/10/09 03:40:33
ditto on re-write.On 2013/09/18 22:40:54, nweiz wr
|
+ |
+ if (index != -1) { |
nweiz
2013/09/18 22:40:54
Use null rather than -1 as a flag value.
terry
2013/10/09 03:40:33
ditto re-wrote not an issue.
On 2013/09/18 22:40:5
|
+ node.declarations.insertAll(index + 1, _newDeclarations); |
+ // Change @include to NoOp so it's processed only once. |
+ node.declarations.replaceRange(index, index + 1, [new NoOp()]); |
+ _foundAndReplaced = true; |
+ } |
+ super.visitDeclarationGroup(node); |
+ } |
+} |
+ |
+class IncludesMissingMixins extends Visitor { |
+ final MixinDefinitions defs; |
+ final Messages _messages; |
+ |
+ IncludesMissingMixins(this.defs, this._messages); |
+ |
+ void visitIncludeDirective(IncludeDirective node) { |
+ if (!defs.map.containsKey(node.name)) { |
+ // @mixin is undefined |
+ _messages.warning("@mixin ${node.name} is undefined.", node.span); |
nweiz
2013/09/18 22:40:54
This should be an error.
terry
2013/10/09 03:40:33
Class removed.
On 2013/09/18 22:40:54, nweiz wrote
|
+ } |
+ |
+ super.visitIncludeDirective(node); |
+ } |
+} |
+ |
+/** |
+ * Remove all @mixin and @include and any NoOp used as placeholder for @include. |
+ */ |
+class RemoveMixinsAndIncludes extends Visitor { |
nweiz
2013/09/18 22:40:54
Don't the other visitors remove the mixins and inc
terry
2013/10/09 03:40:33
class was eliminated when cycles removed.
On 2013/
|
+ RemoveMixinsAndIncludes(); |
+ |
+ bool _nodesToRemove(node) => |
nweiz
2013/09/18 22:40:54
This name sounds like a variable, not a function.
|
+ node is IncludeDirective || node is MixinDefinition || node is NoOp; |
+ |
+ void visitStyleSheet(StyleSheet ss) { |
+ var index = ss.topLevels.length; |
+ while (--index >= 0) { |
nweiz
2013/09/18 22:40:54
It's never clearer to use "--variable" in an expre
|
+ if (_nodesToRemove(ss.topLevels[index])) { |
+ ss.topLevels.removeAt(index); |
+ } |
+ } |
+ |
+ super.visitStyleSheet(ss); |
+ } |
+ |
+ void visitDeclarationGroup(DeclarationGroup node) { |
+ var index = node.declarations.length; |
+ while (--index >= 0) { |
+ if (_nodesToRemove(node.declarations[index])) { |
+ node.declarations.removeAt(index); |
+ } |
+ } |
+ |
+ super.visitDeclarationGroup(node); |
+ } |
+} |