From 1b2719c94c6422112508cfad24bdd9504541c363 Mon Sep 17 00:00:00 2001 From: Laurenz Date: Wed, 29 Jan 2025 15:20:30 +0100 Subject: [PATCH] Resolve bound name of bare import statically (#5773) --- crates/typst-eval/src/import.rs | 49 ++++++++--- crates/typst-ide/src/matchers.rs | 88 +++++++++++-------- crates/typst-library/src/foundations/mod.rs | 4 +- .../typst-library/src/foundations/module.rs | 28 ++++-- crates/typst-library/src/foundations/scope.rs | 9 +- crates/typst-library/src/foundations/value.rs | 15 ++-- crates/typst-library/src/lib.rs | 4 +- crates/typst-library/src/pdf/mod.rs | 2 +- crates/typst-syntax/src/ast.rs | 52 ++++++++++- tests/suite/scripting/import.typ | 66 +++++++++++++- tests/suite/scripting/modules/with space.typ | 1 + 11 files changed, 234 insertions(+), 84 deletions(-) create mode 100644 tests/suite/scripting/modules/with space.typ diff --git a/crates/typst-eval/src/import.rs b/crates/typst-eval/src/import.rs index 2060d25f1..2bbc7e41c 100644 --- a/crates/typst-eval/src/import.rs +++ b/crates/typst-eval/src/import.rs @@ -6,7 +6,7 @@ use typst_library::diag::{ use typst_library::engine::Engine; use typst_library::foundations::{Content, Module, Value}; use typst_library::World; -use typst_syntax::ast::{self, AstNode}; +use typst_syntax::ast::{self, AstNode, BareImportError}; use typst_syntax::package::{PackageManifest, PackageSpec}; use typst_syntax::{FileId, Span, VirtualPath}; @@ -16,11 +16,11 @@ impl Eval for ast::ModuleImport<'_> { type Output = Value; fn eval(self, vm: &mut Vm) -> SourceResult { - let source = self.source(); - let source_span = source.span(); - let mut source = source.eval(vm)?; - let new_name = self.new_name(); - let imports = self.imports(); + let source_expr = self.source(); + let source_span = source_expr.span(); + + let mut source = source_expr.eval(vm)?; + let mut is_str = false; match &source { Value::Func(func) => { @@ -32,6 +32,7 @@ impl Eval for ast::ModuleImport<'_> { Value::Module(_) => {} Value::Str(path) => { source = Value::Module(import(&mut vm.engine, path, source_span)?); + is_str = true; } v => { bail!( @@ -42,9 +43,12 @@ impl Eval for ast::ModuleImport<'_> { } } + // Source itself is imported if there is no import list or a rename. + let bare_name = self.bare_name(); + let new_name = self.new_name(); if let Some(new_name) = new_name { - if let ast::Expr::Ident(ident) = self.source() { - if ident.as_str() == new_name.as_str() { + if let Ok(source_name) = &bare_name { + if source_name == new_name.as_str() { // Warn on `import x as x` vm.engine.sink.warn(warning!( new_name.span(), @@ -58,12 +62,33 @@ impl Eval for ast::ModuleImport<'_> { } let scope = source.scope().unwrap(); - match imports { + match self.imports() { None => { - // Only import here if there is no rename. if new_name.is_none() { - let name: EcoString = source.name().unwrap().into(); - vm.scopes.top.define(name, source); + match self.bare_name() { + // Bare dynamic string imports are not allowed. + Ok(name) + if !is_str || matches!(source_expr, ast::Expr::Str(_)) => + { + if matches!(source_expr, ast::Expr::Ident(_)) { + vm.engine.sink.warn(warning!( + source_expr.span(), + "this import has no effect", + )); + } + vm.scopes.top.define_spanned(name, source, source_span); + } + Ok(_) | Err(BareImportError::Dynamic) => bail!( + source_span, "dynamic import requires an explicit name"; + hint: "you can name the import with `as`" + ), + Err(BareImportError::PathInvalid) => bail!( + source_span, "module name would not be a valid identifier"; + hint: "you can rename the import with `as`", + ), + // Bad package spec would have failed the import already. + Err(BareImportError::PackageInvalid) => unreachable!(), + } } } Some(ast::Imports::Wildcard) => { diff --git a/crates/typst-ide/src/matchers.rs b/crates/typst-ide/src/matchers.rs index b92cbf557..ef8288f2a 100644 --- a/crates/typst-ide/src/matchers.rs +++ b/crates/typst-ide/src/matchers.rs @@ -1,7 +1,7 @@ use ecow::EcoString; use typst::foundations::{Module, Value}; use typst::syntax::ast::AstNode; -use typst::syntax::{ast, LinkedNode, Span, SyntaxKind, SyntaxNode}; +use typst::syntax::{ast, LinkedNode, Span, SyntaxKind}; use crate::{analyze_import, IdeWorld}; @@ -30,38 +30,38 @@ pub fn named_items( if let Some(v) = node.cast::() { let imports = v.imports(); - let source = node - .children() - .find(|child| child.is::()) - .and_then(|source: LinkedNode| { - Some((analyze_import(world, &source)?, source)) - }); - let source = source.as_ref(); + let source = v.source(); + + let source_value = node + .find(source.span()) + .and_then(|source| analyze_import(world, &source)); + let source_value = source_value.as_ref(); + + let module = source_value.and_then(|value| match value { + Value::Module(module) => Some(module), + _ => None, + }); + + let name_and_span = match (imports, v.new_name()) { + // ```plain + // import "foo" as name + // import "foo" as name: .. + // ``` + (_, Some(name)) => Some((name.get().clone(), name.span())), + // ```plain + // import "foo" + // ``` + (None, None) => v.bare_name().ok().map(|name| (name, source.span())), + // ```plain + // import "foo": .. + // ``` + (Some(..), None) => None, + }; // Seeing the module itself. - if let Some((value, source)) = source { - let site = match (imports, v.new_name()) { - // ```plain - // import "foo" as name; - // import "foo" as name: ..; - // ``` - (_, Some(name)) => Some(name.to_untyped()), - // ```plain - // import "foo"; - // ``` - (None, None) => Some(source.get()), - // ```plain - // import "foo": ..; - // ``` - (Some(..), None) => None, - }; - - if let Some((site, value)) = - site.zip(value.clone().cast::().ok()) - { - if let Some(res) = recv(NamedItem::Module(&value, site)) { - return Some(res); - } + if let Some((name, span)) = name_and_span { + if let Some(res) = recv(NamedItem::Module(&name, span, module)) { + return Some(res); } } @@ -75,7 +75,7 @@ pub fn named_items( // import "foo": *; // ``` Some(ast::Imports::Wildcard) => { - if let Some(scope) = source.and_then(|(value, _)| value.scope()) { + if let Some(scope) = source_value.and_then(Value::scope) { for (name, value, span) in scope.iter() { let item = NamedItem::Import(name, span, Some(value)); if let Some(res) = recv(item) { @@ -92,7 +92,7 @@ pub fn named_items( let bound = item.bound_name(); let (span, value) = item.path().iter().fold( - (bound.span(), source.map(|(value, _)| value)), + (bound.span(), source_value), |(span, value), path_ident| { let scope = value.and_then(|v| v.scope()); let span = scope @@ -175,8 +175,8 @@ pub enum NamedItem<'a> { Var(ast::Ident<'a>), /// A function item. Fn(ast::Ident<'a>), - /// A (imported) module item. - Module(&'a Module, &'a SyntaxNode), + /// A (imported) module. + Module(&'a EcoString, Span, Option<&'a Module>), /// An imported item. Import(&'a EcoString, Span, Option<&'a Value>), } @@ -186,7 +186,7 @@ impl<'a> NamedItem<'a> { match self { NamedItem::Var(ident) => ident.get(), NamedItem::Fn(ident) => ident.get(), - NamedItem::Module(value, _) => value.name(), + NamedItem::Module(name, _, _) => name, NamedItem::Import(name, _, _) => name, } } @@ -194,7 +194,7 @@ impl<'a> NamedItem<'a> { pub(crate) fn value(&self) -> Option { match self { NamedItem::Var(..) | NamedItem::Fn(..) => None, - NamedItem::Module(value, _) => Some(Value::Module((*value).clone())), + NamedItem::Module(_, _, value) => value.cloned().map(Value::Module), NamedItem::Import(_, _, value) => value.cloned(), } } @@ -202,7 +202,7 @@ impl<'a> NamedItem<'a> { pub(crate) fn span(&self) -> Span { match *self { NamedItem::Var(name) | NamedItem::Fn(name) => name.span(), - NamedItem::Module(_, site) => site.span(), + NamedItem::Module(_, span, _) => span, NamedItem::Import(_, span, _) => span, } } @@ -356,7 +356,17 @@ mod tests { #[test] fn test_named_items_import() { - test("#import \"foo.typ\": a; #(a);", 2).must_include(["a"]); + test("#import \"foo.typ\"", 2).must_include(["foo"]); + test("#import \"foo.typ\" as bar", 2) + .must_include(["bar"]) + .must_exclude(["foo"]); + } + + #[test] + fn test_named_items_import_items() { + test("#import \"foo.typ\": a; #(a);", 2) + .must_include(["a"]) + .must_exclude(["foo"]); let world = TestWorld::new("#import \"foo.typ\": a.b; #(b);") .with_source("foo.typ", "#import \"a.typ\"") diff --git a/crates/typst-library/src/foundations/mod.rs b/crates/typst-library/src/foundations/mod.rs index 2c3730d53..2921481bc 100644 --- a/crates/typst-library/src/foundations/mod.rs +++ b/crates/typst-library/src/foundations/mod.rs @@ -122,8 +122,8 @@ pub(super) fn define(global: &mut Scope, inputs: Dict, features: &Features) { if features.is_enabled(Feature::Html) { global.define_func::(); } - global.define_module(calc::module()); - global.define_module(sys::module(inputs)); + global.define("calc", calc::module()); + global.define("sys", sys::module(inputs)); } /// Fails with an error. diff --git a/crates/typst-library/src/foundations/module.rs b/crates/typst-library/src/foundations/module.rs index a476d6af1..2001aca16 100644 --- a/crates/typst-library/src/foundations/module.rs +++ b/crates/typst-library/src/foundations/module.rs @@ -32,7 +32,7 @@ use crate::foundations::{repr, ty, Content, Scope, Value}; #[allow(clippy::derived_hash_with_manual_eq)] pub struct Module { /// The module's name. - name: EcoString, + name: Option, /// The reference-counted inner fields. inner: Arc, } @@ -52,14 +52,22 @@ impl Module { /// Create a new module. pub fn new(name: impl Into, scope: Scope) -> Self { Self { - name: name.into(), + name: Some(name.into()), + inner: Arc::new(Repr { scope, content: Content::empty(), file_id: None }), + } + } + + /// Create a new anonymous module without a name. + pub fn anonymous(scope: Scope) -> Self { + Self { + name: None, inner: Arc::new(Repr { scope, content: Content::empty(), file_id: None }), } } /// Update the module's name. pub fn with_name(mut self, name: impl Into) -> Self { - self.name = name.into(); + self.name = Some(name.into()); self } @@ -82,8 +90,8 @@ impl Module { } /// Get the module's name. - pub fn name(&self) -> &EcoString { - &self.name + pub fn name(&self) -> Option<&EcoString> { + self.name.as_ref() } /// Access the module's scope. @@ -105,8 +113,9 @@ impl Module { /// Try to access a definition in the module. pub fn field(&self, name: &str) -> StrResult<&Value> { - self.scope().get(name).ok_or_else(|| { - eco_format!("module `{}` does not contain `{name}`", self.name()) + self.scope().get(name).ok_or_else(|| match &self.name { + Some(module) => eco_format!("module `{module}` does not contain `{name}`"), + None => eco_format!("module does not contain `{name}`"), }) } @@ -131,7 +140,10 @@ impl Debug for Module { impl repr::Repr for Module { fn repr(&self) -> EcoString { - eco_format!("", self.name()) + match &self.name { + Some(module) => eco_format!(""), + None => "".into(), + } } } diff --git a/crates/typst-library/src/foundations/scope.rs b/crates/typst-library/src/foundations/scope.rs index b51f8caaf..99c9a37e6 100644 --- a/crates/typst-library/src/foundations/scope.rs +++ b/crates/typst-library/src/foundations/scope.rs @@ -12,8 +12,8 @@ use typst_utils::Static; use crate::diag::{bail, HintedStrResult, HintedString, StrResult}; use crate::foundations::{ - Element, Func, IntoValue, Module, NativeElement, NativeFunc, NativeFuncData, - NativeType, Type, Value, + Element, Func, IntoValue, NativeElement, NativeFunc, NativeFuncData, NativeType, + Type, Value, }; use crate::Library; @@ -252,11 +252,6 @@ impl Scope { self.define(data.name, Element::from(data)); } - /// Define a module. - pub fn define_module(&mut self, module: Module) { - self.define(module.name().clone(), module); - } - /// Try to access a variable immutably. pub fn get(&self, var: &str) -> Option<&Value> { self.map.get(var).map(Slot::read) diff --git a/crates/typst-library/src/foundations/value.rs b/crates/typst-library/src/foundations/value.rs index 8d9f59332..d99027728 100644 --- a/crates/typst-library/src/foundations/value.rs +++ b/crates/typst-library/src/foundations/value.rs @@ -181,16 +181,6 @@ impl Value { } } - /// The name, if this is a function, type, or module. - pub fn name(&self) -> Option<&str> { - match self { - Self::Func(func) => func.name(), - Self::Type(ty) => Some(ty.short_name()), - Self::Module(module) => Some(module.name()), - _ => None, - } - } - /// Try to extract documentation for the value. pub fn docs(&self) -> Option<&'static str> { match self { @@ -730,6 +720,11 @@ mod tests { assert_eq!(value.into_value().repr(), exp); } + #[test] + fn test_value_size() { + assert!(std::mem::size_of::() <= 32); + } + #[test] fn test_value_debug() { // Primitives. diff --git a/crates/typst-library/src/lib.rs b/crates/typst-library/src/lib.rs index 2ea77eaa5..22f3a62a3 100644 --- a/crates/typst-library/src/lib.rs +++ b/crates/typst-library/src/lib.rs @@ -244,7 +244,7 @@ fn global(math: Module, inputs: Dict, features: &Features) -> Module { self::model::define(&mut global); self::text::define(&mut global); global.reset_category(); - global.define_module(math); + global.define("math", math); self::layout::define(&mut global); self::visualize::define(&mut global); self::introspection::define(&mut global); @@ -253,7 +253,7 @@ fn global(math: Module, inputs: Dict, features: &Features) -> Module { self::pdf::define(&mut global); global.reset_category(); if features.is_enabled(Feature::Html) { - global.define_module(self::html::module()); + global.define("html", self::html::module()); } prelude(&mut global); Module::new("global", global) diff --git a/crates/typst-library/src/pdf/mod.rs b/crates/typst-library/src/pdf/mod.rs index 669835d4c..ec0754631 100644 --- a/crates/typst-library/src/pdf/mod.rs +++ b/crates/typst-library/src/pdf/mod.rs @@ -13,7 +13,7 @@ pub static PDF: Category; /// Hook up the `pdf` module. pub(super) fn define(global: &mut Scope) { global.category(PDF); - global.define_module(module()); + global.define("pdf", module()); } /// Hook up all `pdf` definitions. diff --git a/crates/typst-syntax/src/ast.rs b/crates/typst-syntax/src/ast.rs index 014e8392e..640138e77 100644 --- a/crates/typst-syntax/src/ast.rs +++ b/crates/typst-syntax/src/ast.rs @@ -4,11 +4,14 @@ use std::num::NonZeroUsize; use std::ops::Deref; +use std::path::Path; +use std::str::FromStr; use ecow::EcoString; use unscanny::Scanner; -use crate::{is_newline, Span, SyntaxKind, SyntaxNode}; +use crate::package::PackageSpec; +use crate::{is_ident, is_newline, Span, SyntaxKind, SyntaxNode}; /// A typed AST node. pub trait AstNode<'a>: Sized { @@ -2064,6 +2067,41 @@ impl<'a> ModuleImport<'a> { }) } + /// The name that will be bound for a bare import. This name must be + /// statically known. It can come from: + /// - an identifier + /// - a field access + /// - a string that is a valid file path where the file stem is a valid + /// identifier + /// - a string that is a valid package spec + pub fn bare_name(self) -> Result { + match self.source() { + Expr::Ident(ident) => Ok(ident.get().clone()), + Expr::FieldAccess(access) => Ok(access.field().get().clone()), + Expr::Str(string) => { + let string = string.get(); + let name = if string.starts_with('@') { + PackageSpec::from_str(&string) + .map_err(|_| BareImportError::PackageInvalid)? + .name + } else { + Path::new(string.as_str()) + .file_stem() + .and_then(|path| path.to_str()) + .ok_or(BareImportError::PathInvalid)? + .into() + }; + + if !is_ident(&name) { + return Err(BareImportError::PathInvalid); + } + + Ok(name) + } + _ => Err(BareImportError::Dynamic), + } + } + /// The name this module was assigned to, if it was renamed with `as` /// (`renamed` in `import "..." as renamed`). pub fn new_name(self) -> Option> { @@ -2074,6 +2112,18 @@ impl<'a> ModuleImport<'a> { } } +/// Reasons why a bare name cannot be determined for an import source. +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] +pub enum BareImportError { + /// There is no statically resolvable binding name. + Dynamic, + /// The import source is not a valid path or the path stem not a valid + /// identifier. + PathInvalid, + /// The import source is not a valid package spec. + PackageInvalid, +} + /// The items that ought to be imported from a file. #[derive(Debug, Copy, Clone, Hash)] pub enum Imports<'a> { diff --git a/tests/suite/scripting/import.typ b/tests/suite/scripting/import.typ index 95214db76..03e2efc6b 100644 --- a/tests/suite/scripting/import.typ +++ b/tests/suite/scripting/import.typ @@ -145,6 +145,34 @@ #test(module.item(1, 2), 3) #test(module.push(2), 3) +--- import-from-file-bare-invalid --- +// Error: 9-33 module name would not be a valid identifier +// Hint: 9-33 you can rename the import with `as` +#import "modules/with space.typ" + +--- import-from-file-bare-dynamic --- +// Error: 9-26 dynamic import requires an explicit name +// Hint: 9-26 you can name the import with `as` +#import "mod" + "ule.typ" + +--- import-from-var-bare --- +#let p = "module.typ" +// Error: 9-10 dynamic import requires an explicit name +// Hint: 9-10 you can name the import with `as` +#import p +#test(p.b, 1) + +--- import-from-dict-field-bare --- +#let d = (p: "module.typ") +// Error: 9-12 dynamic import requires an explicit name +// Hint: 9-12 you can name the import with `as` +#import d.p +#test(p.b, 1) + +--- import-from-file-renamed-dynamic --- +#import "mod" + "ule.typ" as mod +#test(mod.b, 1) + --- import-from-file-renamed --- // A renamed module import without items. #import "module.typ" as other @@ -160,6 +188,10 @@ #test(item(1, 2), 3) #test(newname.item(1, 2), 3) +--- import-from-function-scope-bare --- +// Warning: 9-13 this import has no effect +#import enum + --- import-from-function-scope-renamed --- // Renamed module import with function scopes. #import enum as othernum @@ -171,6 +203,23 @@ #import asrt: ne as asne #asne(1, 2) +--- import-from-module-bare --- +#import "modules/chap1.typ" as mymod +// Warning: 9-14 this import has no effect +#import mymod +// The name `chap1` is not bound. +// Error: 2-7 unknown variable: chap1 +#chap1 + +--- import-module-nested --- +#import std.calc: pi +#test(pi, calc.pi) + +--- import-module-nested-bare --- +#import "module.typ" +#import module.chap2 +#test(chap2.name, "Peter") + --- import-module-item-name-mutating --- // Edge case for module access that isn't fixed. #import "module.typ" @@ -214,10 +263,14 @@ // Warning: 31-35 unnecessary import rename to same name #import enum as enum: item as item ---- import-item-rename-unnecessary-but-ok --- -// No warning on a case that isn't obviously pathological +--- import-item-rename-unnecessary-string --- +// Warning: 25-31 unnecessary import rename to same name #import "module.typ" as module +--- import-item-rename-unnecessary-but-ok --- +#import "modul" + "e.typ" as module +#test(module.b, 1) + --- import-from-closure-invalid --- // Can't import from closures. #let f(x) = x @@ -359,6 +412,15 @@ This is never reached. #import "@test/adder:0.1.0" #test(adder.add(2, 8), 10) +--- import-from-package-dynamic --- +// Error: 9-33 dynamic import requires an explicit name +// Hint: 9-33 you can name the import with `as` +#import "@test/" + "adder:0.1.0" + +--- import-from-package-renamed-dynamic --- +#import "@test/" + "adder:0.1.0" as adder +#test(adder.add(2, 8), 10) + --- import-from-package-items --- // Test import with items. #import "@test/adder:0.1.0": add diff --git a/tests/suite/scripting/modules/with space.typ b/tests/suite/scripting/modules/with space.typ new file mode 100644 index 000000000..9138f3c3f --- /dev/null +++ b/tests/suite/scripting/modules/with space.typ @@ -0,0 +1 @@ +// SKIP