Skip to content

Commit

Permalink
fix: lint for undefined global symbols
Browse files Browse the repository at this point in the history 
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
bazel-contrib/vscode-bazel#1 (comment)

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
  • Loading branch information
@hauserx
hauserx committed Nov 19, 2024
1 parent 8d87022 commit 876d930
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 155 deletions.
196 changes: 73 additions & 123 deletions src/bazel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -139,10 +134,6 @@ struct FilesystemCompletionOptions {
pub(crate) struct BazelContext<Client> {
workspaces: RefCell<HashMap<PathBuf, Rc<BazelWorkspace>>>,
query_output_base: Option<PathBuf>,
pub(crate) mode: ContextMode,
pub(crate) print_non_none: bool,
pub(crate) prelude: Vec<FrozenModule>,
pub(crate) module: Option<Module>,
pub(crate) builtin_docs: HashMap<LspUrl, String>,
pub(crate) builtin_symbols: HashMap<String, LspUrl>,
pub(crate) client: Client,
Expand All @@ -151,33 +142,8 @@ pub(crate) struct BazelContext<Client> {
impl<Client: BazelClient> BazelContext<Client> {
pub(crate) fn new(
client: Client,
mode: ContextMode,
print_non_none: bool,
prelude: &[PathBuf],
module: bool,
query_output_base: Option<PathBuf>,
) -> anyhow::Result<Self> {
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::<anyhow::Result<_>>()?;

let module = if module {
Some(Self::new_module(&prelude))
} else {
None
};
let mut builtins: HashMap<LspUrl, Vec<Doc>> = HashMap::new();
let mut builtin_symbols: HashMap<String, LspUrl> = HashMap::new();
for doc in get_registered_starlark_docs() {
Expand All @@ -193,10 +159,6 @@ impl<Client: BazelClient> BazelContext<Client> {
Ok(Self {
workspaces: RefCell::new(HashMap::new()),
query_output_base,
mode,
print_non_none,
prelude,
module,
builtin_docs,
builtin_symbols,
client,
Expand Down Expand Up @@ -231,85 +193,20 @@ impl<Client: BazelClient> BazelContext<Client> {
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<impl Iterator<Item = EvalMessage>> {
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<impl Iterator<Item = EvalMessage>> {
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<impl Iterator<Item = EvalMessage>> {
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<Item = EvalMessage> {
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.
Expand Down Expand Up @@ -358,15 +255,15 @@ impl<Client: BazelClient> BazelContext<Client> {
}
}

pub(crate) fn file_with_contents(
fn file_with_contents(
&self,
filename: &str,
uri: &LspUrl,
content: String,
) -> EvalResult<impl Iterator<Item = EvalMessage>> {
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),
)
}
Expand Down Expand Up @@ -594,8 +491,10 @@ impl<Client: BazelClient> BazelContext<Client> {
self.client.build_language(&workspace)
}

fn try_get_environment(&self, uri: &LspUrl) -> anyhow::Result<DocModule> {
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);

Expand All @@ -605,9 +504,18 @@ impl<Client: BazelClient> BazelContext<Client> {

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<DocModule> {
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)))
Expand All @@ -618,14 +526,32 @@ impl<Client: BazelClient> BazelContext<Client> {
members,
})
}

fn get_bazel_globals_names(&self, uri: &LspUrl) -> HashSet<String> {
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<Client: BazelClient> LspContext for BazelContext<Client> {
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,
Expand Down Expand Up @@ -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")));

Expand Down Expand Up @@ -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(())
}
}
43 changes: 43 additions & 0 deletions src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Item = (String, DocMember)> + 'a {
Expand Down
2 changes: 2 additions & 0 deletions src/builtin/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
])
16 changes: 2 additions & 14 deletions src/eval.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T: Iterator<Item = EvalMessage>> {
/// 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<AstModule>,
}


pub(crate) fn globals() -> Globals {
Globals::extended_internal()
}

pub(crate) fn dialect() -> Dialect {
Dialect::Extended
}
Loading

0 comments on commit 876d930

Please sign in to comment.