From 876d930d1ec5bde0e814cfab2e3f5882713ac464 Mon Sep 17 00:00:00 2001 From: Grzegorz Lukasik Date: Sun, 17 Nov 2024 18:54:32 +0100 Subject: [PATCH] fix: lint for undefined global symbols Previous implementation was passing empty prelude list to lint. The starlark-rust does not create warnings for undefined global symbols in such case. In general, for bazel we do not have prelude, just a list of global symbols. Needed to add manually some missing symbols, because bazel does not report all global symbols in `bazel info build-language`, see https://github.com/bazel-contrib/vscode-bazel/issues/1#issuecomment-2036369868 In the process: - Removed unused prelude. Looks it was carried over with migrating from starlark-rust, but prelude is not used in case of bazel. - Removed ContextMode run - bazel lsp is not able to actually run any starlark, those seems to be leftovers from starlark-rust --- src/bazel.rs | 196 +++++++++++++++++--------------------------- src/builtin.rs | 43 ++++++++++ src/builtin/BUILD | 2 + src/eval.rs | 16 +--- src/main.rs | 5 -- src/test_fixture.rs | 13 --- 6 files changed, 120 insertions(+), 155 deletions(-) diff --git a/src/bazel.rs b/src/bazel.rs index bd1899d..9c0ec43 100644 --- a/src/bazel.rs +++ b/src/bazel.rs @@ -43,10 +43,7 @@ use starlark::docs::render_docs_as_code; use starlark::docs::Doc; use starlark::docs::DocItem; use starlark::docs::DocModule; -use starlark::environment::FrozenModule; -use starlark::environment::Module; use starlark::errors::EvalMessage; -use starlark::eval::Evaluator; use starlark::syntax::AstModule; use starlark_lsp::completion::StringCompletionResult; use starlark_lsp::completion::StringCompletionType; @@ -59,8 +56,6 @@ use starlark_lsp::server::StringLiteralResult; use crate::builtin; use crate::client::BazelClient; use crate::eval::dialect; -use crate::eval::globals; -use crate::eval::ContextMode; use crate::eval::EvalResult; use crate::file_type::FileType; use crate::label::Label; @@ -139,10 +134,6 @@ struct FilesystemCompletionOptions { pub(crate) struct BazelContext { workspaces: RefCell>>, query_output_base: Option, - pub(crate) mode: ContextMode, - pub(crate) print_non_none: bool, - pub(crate) prelude: Vec, - pub(crate) module: Option, pub(crate) builtin_docs: HashMap, pub(crate) builtin_symbols: HashMap, pub(crate) client: Client, @@ -151,33 +142,8 @@ pub(crate) struct BazelContext { impl BazelContext { pub(crate) fn new( client: Client, - mode: ContextMode, - print_non_none: bool, - prelude: &[PathBuf], - module: bool, query_output_base: Option, ) -> anyhow::Result { - let globals = globals(); - let prelude: Vec<_> = prelude - .iter() - .map(|x| { - let env = Module::new(); - { - let mut eval = Evaluator::new(&env); - let module = AstModule::parse_file(x, &dialect()) - .map_err(starlark::Error::into_anyhow)?; - eval.eval_module(module, &globals) - .map_err(starlark::Error::into_anyhow)?; - } - env.freeze() - }) - .collect::>()?; - - let module = if module { - Some(Self::new_module(&prelude)) - } else { - None - }; let mut builtins: HashMap> = HashMap::new(); let mut builtin_symbols: HashMap = HashMap::new(); for doc in get_registered_starlark_docs() { @@ -193,10 +159,6 @@ impl BazelContext { Ok(Self { workspaces: RefCell::new(HashMap::new()), query_output_base, - mode, - print_non_none, - prelude, - module, builtin_docs, builtin_symbols, client, @@ -231,85 +193,20 @@ impl BazelContext { LspUrl::try_from(url).unwrap() } - fn new_module(prelude: &[FrozenModule]) -> Module { - let module = Module::new(); - for p in prelude { - module.import_public_symbols(p); - } - module - } + fn go(&self, uri: &LspUrl, ast: AstModule) -> EvalResult> { + let globals = self.get_bazel_globals_names(uri); + + let warnings = ast + .lint(Some(globals).as_ref()) + .into_iter() + .map(EvalMessage::from); - fn go(&self, file: &str, ast: AstModule) -> EvalResult> { - let mut warnings = Either::Left(iter::empty()); - let mut errors = Either::Left(iter::empty()); - let final_ast = match self.mode { - ContextMode::Check => { - warnings = Either::Right(self.check(&ast)); - Some(ast) - } - ContextMode::Run => { - errors = Either::Right(self.run(file, ast).messages); - None - } - }; EvalResult { - messages: warnings.chain(errors), - ast: final_ast, + messages: warnings, + ast: Some(ast), } } - fn run(&self, file: &str, ast: AstModule) -> EvalResult> { - let new_module; - let module = match self.module.as_ref() { - Some(module) => module, - None => { - new_module = Self::new_module(&self.prelude); - &new_module - } - }; - let mut eval = Evaluator::new(module); - eval.enable_terminal_breakpoint_console(); - let globals = globals(); - Self::err( - file, - eval.eval_module(ast, &globals) - .map(|v| { - if self.print_non_none && !v.is_none() { - println!("{}", v); - } - EvalResult { - messages: iter::empty(), - ast: None, - } - }) - .map_err(Into::into), - ) - } - - fn check(&self, module: &AstModule) -> impl Iterator { - let globals = if self.prelude.is_empty() { - None - } else { - let mut globals = HashSet::new(); - for modu in &self.prelude { - for name in modu.names() { - globals.insert(name.as_str().to_owned()); - } - } - - for global_symbol in self.builtin_symbols.keys() { - globals.insert(global_symbol.to_owned()); - } - - Some(globals) - }; - - module - .lint(globals.as_ref()) - .into_iter() - .map(EvalMessage::from) - } - /// Gets the possibly-cached workspace for a directory, or creates a new one if it doesn't exist. /// If the workspace is not given, it is inferred based on the current file. /// Returns None if a workspace cannot be found. @@ -358,15 +255,15 @@ impl BazelContext { } } - pub(crate) fn file_with_contents( + fn file_with_contents( &self, - filename: &str, + uri: &LspUrl, content: String, ) -> EvalResult> { Self::err( - filename, - AstModule::parse(filename, content, &dialect()) - .map(|module| self.go(filename, module)) + &uri.path().to_string_lossy(), + AstModule::parse(&uri.path().to_string_lossy(), content, &dialect()) + .map(|module| self.go(uri, module)) .map_err(Into::into), ) } @@ -594,8 +491,10 @@ impl BazelContext { self.client.build_language(&workspace) } - fn try_get_environment(&self, uri: &LspUrl) -> anyhow::Result { - let file_type = FileType::from_lsp_url(uri); + /// Returns protos for bazel globals (like int, str, dir; but also e.g. cc_library, alias, + /// test_suite etc.). + // TODO: Consider caching this + fn get_bazel_globals(&self, uri: &LspUrl) -> (builtin::BuildLanguage, builtin::Builtins) { let language_proto = self.get_build_language_proto(uri); @@ -605,9 +504,18 @@ impl BazelContext { let language = builtin::BuildLanguage::decode(&language_proto[..]).unwrap(); + // TODO: builtins are also dependent on bazel version, but there is no way to obtain those, + // see https://github.com/bazel-contrib/vscode-bazel/issues/1. let builtins_proto = include_bytes!("builtin/builtin.pb"); let builtins = builtin::Builtins::decode(&builtins_proto[..]).unwrap(); + (language, builtins) + } + + fn try_get_environment(&self, uri: &LspUrl) -> anyhow::Result { + let file_type = FileType::from_lsp_url(uri); + let (language, builtins) = self.get_bazel_globals(uri); + let members: SmallMap<_, _> = builtin::build_language_to_doc_members(&language) .chain(builtin::builtins_to_doc_members(&builtins, file_type)) .map(|(name, member)| (name, DocItem::Member(member))) @@ -618,14 +526,32 @@ impl BazelContext { members, }) } + + fn get_bazel_globals_names(&self, uri: &LspUrl) -> HashSet { + let (language, builtins) = self.get_bazel_globals(uri); + + language.rule + .iter() + .map(|rule| rule.name.clone()) + .chain( + builtins.global + .iter() + .map(|global| global.name.clone()) + ) + .chain( + builtin::MISSING_GLOBALS.iter() + .map(|missing| missing.to_string()) + ) + .collect() + } } impl LspContext for BazelContext { fn parse_file_with_contents(&self, uri: &LspUrl, content: String) -> LspEvalResult { match uri { - LspUrl::File(uri) => { + LspUrl::File(_) => { let EvalResult { messages, ast } = - self.file_with_contents(&uri.to_string_lossy(), content); + self.file_with_contents(uri, content); LspEvalResult { diagnostics: messages.map(eval_message_to_lsp_diagnostic).collect(), ast, @@ -1202,9 +1128,9 @@ mod tests { module.members.iter().any(|(member, _)| member == value) } - // let module = context.get_environment(&LspUrl::File(PathBuf::from("/foo/bar.bzl"))); + let module = context.get_environment(&LspUrl::File(PathBuf::from("/foo/bar.bzl"))); - // assert!(module_contains(&module, "cc_library")); + assert!(module_contains(&module, "cc_library")); let module = context.get_environment(&LspUrl::File(PathBuf::from("/foo/bar/BUILD"))); @@ -1333,4 +1259,28 @@ mod tests { Ok(()) } + + #[test] + fn reports_undefined_global_symbols() -> anyhow::Result<()> { + let fixture = TestFixture::new("simple")?; + let context = fixture.context()?; + + let result = context.parse_file_with_contents( + &LspUrl::File(PathBuf::from("/foo.bzl")), + " +test_suite(name='my_test_suite'); + +unknown_global_function(42); + +a=int(7); + +register_toolchains([':my_toolchain']); +".to_string() + ); + + assert_eq!(1, result.diagnostics.len()); + assert_eq!("Use of undefined variable `unknown_global_function`", result.diagnostics[0].message); + + Ok(()) + } } diff --git a/src/builtin.rs b/src/builtin.rs index 0527750..b493b68 100644 --- a/src/builtin.rs +++ b/src/builtin.rs @@ -7,6 +7,49 @@ use starlark::{ use crate::file_type::FileType; +/// Names of globals missing in builtins reported by bazel. +/// See e.g. https://github.com/bazel-contrib/vscode-bazel/issues/1#issuecomment-2036369868 +pub static MISSING_GLOBALS: &'static[&'static str] = &[ + // All values from https://bazel.build/rules/lib/globals/workspace + "bind", + "register_execution_platforms", + "register_toolchains", + "workspace", + + // Values from https://bazel.build/rules/lib/globals/module + "archive_override", + "bazel_dep", + "git_override", + "include", + "inject_repo", + "local_path_override", + "module", + "multiple_version_override", + "override_repo", + "register_execution_platforms", + "register_toolchains", + "single_version_override", + "use_extension", + "use_repo", + "use_repo_rule", + + // Missing values from https://bazel.build/rules/lib/globals/build + "package", + "repo_name", + + // Missing values from https://bazel.build/rules/lib/globals/bzl + "exec_transition", + "module_extension", + "repository_rule", + "tag_class", + + // Marked as not documented on https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/packages/BuildGlobals.java + "licenses", + "environment_group", + // Removed in https://github.com/bazelbuild/bazel/commit/5ade9da5de25bc93d0ec79faea8f08a54e5b9a68 + "distribs", +]; + pub fn build_language_to_doc_members<'a>( build_language: &'a BuildLanguage, ) -> impl Iterator + 'a { diff --git a/src/builtin/BUILD b/src/builtin/BUILD index 303179b..f8947c6 100644 --- a/src/builtin/BUILD +++ b/src/builtin/BUILD @@ -16,5 +16,7 @@ exports_files([ # Built from the bazel repo using: # `bazel build //src/main/java/com/google/devtools/build/lib:gen_api_proto` "builtin.pb", + # Obtained from bazel instance with: + # `bazel info build-language` "default_build_language.pb", ]) diff --git a/src/eval.rs b/src/eval.rs index 0443cd2..dbcb214 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -15,31 +15,19 @@ * limitations under the License. */ -use starlark::environment::Globals; use starlark::errors::EvalMessage; use starlark::syntax::AstModule; use starlark::syntax::Dialect; -#[derive(Debug)] -pub(crate) enum ContextMode { - Check, - Run, -} - -/// The outcome of evaluating (checking, parsing or running) given starlark code. +/// The outcome of evaluating (checking or parsing) given starlark code. pub(crate) struct EvalResult> { /// The diagnostic and error messages from evaluating a given piece of starlark code. pub messages: T, - /// If the code is only parsed, not run, and there were no errors, this will contain - /// the parsed module. Otherwise, it will be `None` + /// If there were no errors, this will contain the parsed module. Otherwise, it will be `None` pub ast: Option, } -pub(crate) fn globals() -> Globals { - Globals::extended_internal() -} - pub(crate) fn dialect() -> Dialect { Dialect::Extended } diff --git a/src/main.rs b/src/main.rs index d3e7400..6d969df 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,6 @@ use std::{env, path::PathBuf}; use bazel::BazelContext; use clap::Parser; use client::BazelCli; -use eval::ContextMode; #[derive(Parser, Debug)] #[command(author, version, about, long_about = None)] @@ -51,10 +50,6 @@ fn main() -> anyhow::Result<()> { let ctx = BazelContext::new( BazelCli::new(args.bazel), - ContextMode::Check, - true, - &[], - true, query_output_base, )?; diff --git a/src/test_fixture.rs b/src/test_fixture.rs index 4145b34..55df414 100644 --- a/src/test_fixture.rs +++ b/src/test_fixture.rs @@ -9,7 +9,6 @@ use anyhow::anyhow; use crate::{ bazel::BazelContext, client::{BazelInfo, MockBazel, ProfilingClient}, - eval::ContextMode, }; pub struct TestFixture { @@ -59,20 +58,12 @@ impl TestFixture { queries: HashMap::new(), repo_mappings: HashMap::new(), }, - mode: ContextMode::Check, - print_non_none: true, - prelude: Vec::new(), - module: true, }) } } pub(crate) struct ContextBuilder { client: MockBazel, - mode: ContextMode, - print_non_none: bool, - prelude: Vec, - module: bool, } impl ContextBuilder { @@ -97,10 +88,6 @@ impl ContextBuilder { pub(crate) fn build(self) -> anyhow::Result>> { BazelContext::new( ProfilingClient::new(self.client), - self.mode, - self.print_non_none, - &self.prelude, - self.module, None, ) }