From d0d9459d8e3b1445bc88cd3e6b3edc39e0e96af6 Mon Sep 17 00:00:00 2001 From: connorskees Date: Wed, 18 Jan 2023 05:55:55 +0000 Subject: [PATCH] improve compressed output for selectors and colors --- crates/compiler/src/ast/stmt.rs | 5 +- crates/compiler/src/evaluate/env.rs | 120 +++++++++++++++++++++++- crates/compiler/src/evaluate/visitor.rs | 60 +++++++++++- crates/compiler/src/parse/stylesheet.rs | 11 +++ crates/compiler/src/serializer.rs | 17 +++- crates/lib/tests/compressed.rs | 2 - crates/lib/tests/imports.rs | 20 ++++ crates/lib/tests/use.rs | 90 ++++++++++++++++++ 8 files changed, 312 insertions(+), 13 deletions(-) diff --git a/crates/compiler/src/ast/stmt.rs b/crates/compiler/src/ast/stmt.rs index 950bd97..537b323 100644 --- a/crates/compiler/src/ast/stmt.rs +++ b/crates/compiler/src/ast/stmt.rs @@ -552,8 +552,9 @@ pub(crate) struct StyleSheet { pub body: Vec, pub url: PathBuf, pub is_plain_css: bool, - pub uses: Vec, - pub forwards: Vec, + /// Array of indices into body + pub uses: Vec, + pub forwards: Vec, } impl StyleSheet { diff --git a/crates/compiler/src/evaluate/env.rs b/crates/compiler/src/evaluate/env.rs index d5fbe68..2e612c8 100644 --- a/crates/compiler/src/evaluate/env.rs +++ b/crates/compiler/src/evaluate/env.rs @@ -1,8 +1,8 @@ use codemap::{Span, Spanned}; use crate::{ - ast::{AstForwardRule, Mixin}, - builtin::modules::{ForwardedModule, Module, Modules}, + ast::{AstForwardRule, Configuration, Mixin}, + builtin::modules::{ForwardedModule, Module, ModuleScope, Modules}, common::Identifier, error::SassResult, selector::ExtensionStore, @@ -42,6 +42,122 @@ impl Environment { } } + pub fn for_import(&self) -> Self { + Self { + scopes: self.scopes.new_closure(), + modules: Arc::new(RefCell::new(Modules::new())), + global_modules: Vec::new(), + content: self.content.as_ref().map(Arc::clone), + forwarded_modules: Arc::clone(&self.forwarded_modules), + } + } + + pub fn to_dummy_module(&self, span: Span) -> Module { + Module::Environment { + scope: ModuleScope::new(), + upstream: Vec::new(), + extension_store: ExtensionStore::new(span), + env: self.clone(), + } + } + + pub fn import_forwards(&mut self, _env: Module) { + // if (module is _EnvironmentModule) { + // var forwarded = module._environment._forwardedModules; + // if (forwarded == null) return; + + // // Omit modules from [forwarded] that are already globally available and + // // forwarded in this module. + // var forwardedModules = _forwardedModules; + // if (forwardedModules != null) { + // forwarded = { + // for (var entry in forwarded.entries) + // if (!forwardedModules.containsKey(entry.key) || + // !_globalModules.containsKey(entry.key)) + // entry.key: entry.value, + // }; + // } else { + // forwardedModules = _forwardedModules ??= {}; + // } + + // var forwardedVariableNames = + // forwarded.keys.expand((module) => module.variables.keys).toSet(); + // var forwardedFunctionNames = + // forwarded.keys.expand((module) => module.functions.keys).toSet(); + // var forwardedMixinNames = + // forwarded.keys.expand((module) => module.mixins.keys).toSet(); + + // if (atRoot) { + // // Hide members from modules that have already been imported or + // // forwarded that would otherwise conflict with the @imported members. + // for (var entry in _importedModules.entries.toList()) { + // var module = entry.key; + // var shadowed = ShadowedModuleView.ifNecessary(module, + // variables: forwardedVariableNames, + // mixins: forwardedMixinNames, + // functions: forwardedFunctionNames); + // if (shadowed != null) { + // _importedModules.remove(module); + // if (!shadowed.isEmpty) _importedModules[shadowed] = entry.value; + // } + // } + + // for (var entry in forwardedModules.entries.toList()) { + // var module = entry.key; + // var shadowed = ShadowedModuleView.ifNecessary(module, + // variables: forwardedVariableNames, + // mixins: forwardedMixinNames, + // functions: forwardedFunctionNames); + // if (shadowed != null) { + // forwardedModules.remove(module); + // if (!shadowed.isEmpty) forwardedModules[shadowed] = entry.value; + // } + // } + + // _importedModules.addAll(forwarded); + // forwardedModules.addAll(forwarded); + // } else { + // (_nestedForwardedModules ??= + // List.generate(_variables.length - 1, (_) => [])) + // .last + // .addAll(forwarded.keys); + // } + + // // Remove existing member definitions that are now shadowed by the + // // forwarded modules. + // for (var variable in forwardedVariableNames) { + // _variableIndices.remove(variable); + // _variables.last.remove(variable); + // _variableNodes.last.remove(variable); + // } + // for (var function in forwardedFunctionNames) { + // _functionIndices.remove(function); + // _functions.last.remove(function); + // } + // for (var mixin in forwardedMixinNames) { + // _mixinIndices.remove(mixin); + // _mixins.last.remove(mixin); + // } + // } + // todo!() + } + + pub fn to_implicit_configuration(&self) -> Configuration { + // var configuration = {}; + // for (var i = 0; i < _variables.length; i++) { + // var values = _variables[i]; + // var nodes = _variableNodes[i]; + // for (var entry in values.entries) { + // // Implicit configurations are never invalid, making [configurationSpan] + // // unnecessary, so we pass null here to avoid having to compute it. + // configuration[entry.key] = + // ConfiguredValue.implicit(entry.value, nodes[entry.key]!); + // } + // } + // return Configuration.implicit(configuration); + todo!() + } + pub fn forward_module(&mut self, module: Arc>, rule: AstForwardRule) { let view = ForwardedModule::if_necessary(module, rule); (*self.forwarded_modules).borrow_mut().push(view); diff --git a/crates/compiler/src/evaluate/visitor.rs b/crates/compiler/src/evaluate/visitor.rs index 48683ff..43ecd00 100644 --- a/crates/compiler/src/evaluate/visitor.rs +++ b/crates/compiler/src/evaluate/visitor.rs @@ -767,8 +767,9 @@ impl<'a> Visitor<'a> { || path_buf.extension() == Some(OsStr::new("css")) { let extension = path_buf.extension().unwrap(); - try_path!(path.with_extension(format!(".import{}", extension.to_str().unwrap()))); - try_path!(path); + try_path!(path_buf.with_extension(format!(".import{}", extension.to_str().unwrap()))); + try_path!(path_buf); + // todo: consider load paths return None; } @@ -882,9 +883,62 @@ impl<'a> Visitor<'a> { return Ok(()); } + // todo: + let loads_user_defined_modules = true; + // this todo should be unreachable, as we currently do not push // to stylesheet.uses or stylesheet.forwards - todo!() + // let mut children = Vec::new(); + let env = self.env.for_import(); + + self.with_environment::, _>(env.clone(), |visitor| { + let old_parent = visitor.parent; + let old_configuration = Arc::clone(&visitor.configuration); + + if loads_user_defined_modules { + visitor.parent = Some(CssTree::ROOT); + } + + // This configuration is only used if it passes through a `@forward` + // rule, so we avoid creating unnecessary ones for performance reasons. + if !stylesheet.forwards.is_empty() { + visitor.configuration = Arc::new(RefCell::new(env.to_implicit_configuration())); + } + + visitor.visit_stylesheet(stylesheet)?; + + if loads_user_defined_modules { + visitor.parent = old_parent; + } + visitor.configuration = old_configuration; + + Ok(()) + })?; + + // Create a dummy module with empty CSS and no extensions to make forwarded + // members available in the current import context and to combine all the + // CSS from modules used by [stylesheet]. + let module = env.to_dummy_module(self.span_before); + self.env.import_forwards(module); + + if loads_user_defined_modules { + // if (module.transitivelyContainsCss) { + // // If any transitively used module contains extensions, we need to + // // clone all modules' CSS. Otherwise, it's possible that they'll be + // // used or imported from another location that shouldn't have the same + // // extensions applied. + // await _combineCss(module, + // clone: module.transitivelyContainsExtensions) + // .accept(this); + // } + + // var visitor = _ImportedCssVisitor(this); + // for (var child in children) { + // child.accept(visitor); + // } + } + + Ok(()) } fn visit_static_import_rule(&mut self, static_import: AstPlainCssImport) -> SassResult<()> { diff --git a/crates/compiler/src/parse/stylesheet.rs b/crates/compiler/src/parse/stylesheet.rs index 5439961..8c3a1a8 100644 --- a/crates/compiler/src/parse/stylesheet.rs +++ b/crates/compiler/src/parse/stylesheet.rs @@ -202,6 +202,17 @@ pub(crate) trait StylesheetParser<'a>: BaseParser<'a> + Sized { Ok(Some(parser.parse_statement()?)) })?; + for (idx, child) in style_sheet.body.iter().enumerate() { + match child { + AstStmt::VariableDecl(_) | AstStmt::LoudComment(_) | AstStmt::SilentComment(_) => { + continue + } + AstStmt::Use(..) => style_sheet.uses.push(idx), + AstStmt::Forward(..) => style_sheet.forwards.push(idx), + _ => break, + } + } + Ok(style_sheet) } diff --git a/crates/compiler/src/serializer.rs b/crates/compiler/src/serializer.rs index 22f4473..ae10f38 100644 --- a/crates/compiler/src/serializer.rs +++ b/crates/compiler/src/serializer.rs @@ -285,7 +285,7 @@ impl<'a> Serializer<'a> { } else { self.buffer.push(b','); if complex.line_break { - self.buffer.push(b'\n'); + self.write_newline(); } else { self.write_optional_space(); } @@ -294,6 +294,12 @@ impl<'a> Serializer<'a> { } } + fn write_newline(&mut self) { + if !self.options.is_compressed() { + self.buffer.push(b'\n'); + } + } + fn write_comma_separator(&mut self) { self.buffer.push(b','); self.write_optional_space(); @@ -398,13 +404,16 @@ impl<'a> Serializer<'a> { } self.write_float(color.red().0); - self.buffer.extend_from_slice(b", "); + self.buffer.extend_from_slice(b","); + self.write_optional_space(); self.write_float(color.green().0); - self.buffer.extend_from_slice(b", "); + self.buffer.extend_from_slice(b","); + self.write_optional_space(); self.write_float(color.blue().0); if !is_opaque { - self.buffer.extend_from_slice(b", "); + self.buffer.extend_from_slice(b","); + self.write_optional_space(); self.write_float(color.alpha().0); } diff --git a/crates/lib/tests/compressed.rs b/crates/lib/tests/compressed.rs index 0f163eb..8f32de9 100644 --- a/crates/lib/tests/compressed.rs +++ b/crates/lib/tests/compressed.rs @@ -26,7 +26,6 @@ test!( grass::Options::default().style(grass::OutputStyle::Compressed) ); test!( - #[ignore = "regress selector compression"] compresses_selector_with_newline_after_comma, "a,\nb {\n color: red;\n}\n", "a,b{color:red}", @@ -100,7 +99,6 @@ test!( grass::Options::default().style(grass::OutputStyle::Compressed) ); test!( - #[ignore = "we do not support compressed colors"] removes_leading_zero_in_number_under_1_in_rgba_alpha_channel, "a {\n color: rgba(1, 1, 1, 0.5);\n}\n", "a{color:rgba(1,1,1,.5)}", diff --git a/crates/lib/tests/imports.rs b/crates/lib/tests/imports.rs index 67c45c0..d13d8ba 100644 --- a/crates/lib/tests/imports.rs +++ b/crates/lib/tests/imports.rs @@ -483,6 +483,26 @@ fn chained_imports_in_directory() { &grass::from_string(input.to_string(), &grass::Options::default()).expect(input) ); } + +#[test] +fn explicit_file_extension_import_inside_index_file() { + let input = + "@import \"explicit_file_extension_import_inside_index_file\";\na {\n color: $a;\n}"; + tempfile!( + "_a.scss", + "$a: red;", + dir = "explicit_file_extension_import_inside_index_file" + ); + tempfile!( + "_index.scss", + "@import \"a.scss\";", + dir = "explicit_file_extension_import_inside_index_file" + ); + assert_eq!( + "a {\n color: red;\n}\n", + &grass::from_string(input.to_string(), &grass::Options::default()).expect(input) + ); +} error!( // note: dart-sass error is "expected more input." missing_input_after_import, diff --git a/crates/lib/tests/use.rs b/crates/lib/tests/use.rs index 6b26b8d..d265b77 100644 --- a/crates/lib/tests/use.rs +++ b/crates/lib/tests/use.rs @@ -702,4 +702,94 @@ fn use_variable_declared_in_two_modules() { ); } +#[test] +fn import_module_using_same_builtin_module() { + let mut fs = TestFs::new(); + + fs.add_file( + "_a.scss", + r#" + @use "sass:meta"; + "#, + ); + + fs.add_file( + "_b.scss", + r#" + $a: red; + "#, + ); + + let input = r#" + @use "sass:meta"; + @import "a"; + "#; + + assert_eq!( + "", + &grass::from_string(input.to_string(), &grass::Options::default().fs(&fs)).expect(input) + ); +} + +#[test] +fn import_module_using_same_builtin_module_has_styles() { + let mut fs = TestFs::new(); + + fs.add_file( + "_a.scss", + r#" + @use "sass:meta"; + + a { + color: red; + } + "#, + ); + + fs.add_file( + "_b.scss", + r#" + $a: red; + "#, + ); + + let input = r#" + @use "sass:meta"; + @import "a"; + "#; + + assert_eq!( + "a {\n color: red;\n}\n", + &grass::from_string(input.to_string(), &grass::Options::default().fs(&fs)).expect(input) + ); +} + +#[test] +#[ignore = "we don't hermetically evaluate @extend"] +fn use_module_with_extend() { + let mut fs = TestFs::new(); + + fs.add_file( + "_a.scss", + r#" + a { + @extend b; + } + "#, + ); + + let input = r#" + @use "a"; + b { + color: red; + } + "#; + + assert_err!( + input, + "Error: The target selector was not found.", + grass::Options::default().fs(&fs) + ); +} + // todo: refactor these tests to use testfs where possible