Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: python imports #438

Merged
merged 9 commits into from
Jun 2, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
;; ^^^^^^^^^^^^^^^^

global FILE_PATH
global ROOT_PATH = ""
global ROOT_NODE
global JUMP_TO_SCOPE_NODE

Expand Down Expand Up @@ -272,7 +273,9 @@ inherit .parent_module
node grandparent_module_ref_node
var grandparent_module_ref = grandparent_module_ref_node

scan FILE_PATH {
; get the file path relative to the root path
let rel_path = (replace FILE_PATH ROOT_PATH "")
scan rel_path {
"([^/]+)/"
{
node def_dot
Expand Down
21 changes: 21 additions & 0 deletions languages/tree-sitter-stack-graphs-python/test/root_path.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# ------ path: foo/bar/module.py -----------#
# ------ global: ROOT_PATH=foo/bar -----------#

foo = 42

# ------ path: foo/bar/baz/module.py -----------#
# ------ global: ROOT_PATH=foo/bar -----------#

bar = "hello"

# ------ path: foo/bar/main.py -------------#
# ------ global: ROOT_PATH=foo/bar -----------#

from module import foo
from baz.module import bar

print(foo)
# ^ defined: 4, 14

print(bar)
# ^ defined: 9, 15
12 changes: 11 additions & 1 deletion tree-sitter-stack-graphs/src/cli/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ use crate::BuildError;
use crate::CancelAfterDuration;
use crate::CancellationFlag;
use crate::NoCancellation;
use crate::{FILE_PATH_VAR, ROOT_PATH_VAR};

#[derive(Args)]
pub struct IndexArgs {
Expand Down Expand Up @@ -429,7 +430,16 @@ impl<'a> Indexer<'a> {
) -> std::result::Result<(), BuildErrorWithSource<'b>> {
let relative_source_path = source_path.strip_prefix(source_root).unwrap();
if let Some(lc) = lcs.primary {
let globals = Variables::new();
let mut globals = Variables::new();

globals
.add(FILE_PATH_VAR.into(), source_path.to_str().unwrap().into())
.expect("failed to add file path variable");

globals
.add(ROOT_PATH_VAR.into(), source_root.to_str().unwrap().into())
.expect("failed to add root path variable");

lc.sgl
.build_stack_graph_into(graph, file, source, &globals, cancellation_flag)
.map_err(|inner| BuildErrorWithSource {
Expand Down
10 changes: 10 additions & 0 deletions tree-sitter-stack-graphs/src/cli/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use crate::test::Test;
use crate::test::TestResult;
use crate::CancelAfterDuration;
use crate::CancellationFlag;
use crate::FILE_PATH_VAR;

#[derive(Args)]
#[clap(after_help = r#"PATH SPECIFICATIONS:
Expand Down Expand Up @@ -316,6 +317,15 @@ impl TestArgs {
&mut Some(test_fragment.source.as_ref()),
)? {
globals.clear();

// Add FILE_PATH to variables
globals
.add(
FILE_PATH_VAR.into(),
test_fragment.path.to_str().unwrap().into(),
)
.expect("failed to add file path variable");

test_fragment.add_globals_to(&mut globals);
lc.sgl.build_stack_graph_into(
&mut test.graph,
Expand Down
21 changes: 12 additions & 9 deletions tree-sitter-stack-graphs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,16 @@ static SCOPE_ATTRS: Lazy<HashSet<&'static str>> =
static PRECEDENCE_ATTR: &'static str = "precedence";

// Global variables
static ROOT_NODE_VAR: &'static str = "ROOT_NODE";
static JUMP_TO_SCOPE_NODE_VAR: &'static str = "JUMP_TO_SCOPE_NODE";
static FILE_PATH_VAR: &'static str = "FILE_PATH";
/// Name of the variable used to pass the root node.
pub const ROOT_NODE_VAR: &'static str = "ROOT_NODE";
/// Name of the variable used to pass the jump-to-scope node.
pub const JUMP_TO_SCOPE_NODE_VAR: &'static str = "JUMP_TO_SCOPE_NODE";
/// Name of the variable used to pass the file path.
/// If a root path is given, it should be a descendant the root path.
pub const FILE_PATH_VAR: &'static str = "FILE_PATH";
/// Name of the variable used to pass the root path.
/// If given, should be an ancestor of the file path.
pub const ROOT_PATH_VAR: &'static str = "ROOT_PATH";

/// Holds information about how to construct stack graphs for a particular language.
pub struct StackGraphLanguage {
Expand Down Expand Up @@ -635,22 +642,18 @@ impl<'a> Builder<'a> {
let tree = parse_errors.into_tree();

let mut globals = Variables::nested(globals);

if globals.get(&ROOT_NODE_VAR.into()).is_none() {
let root_node = self.inject_node(NodeID::root());
globals
.add(ROOT_NODE_VAR.into(), root_node.into())
.expect("Failed to set ROOT_NODE");
}

let jump_to_scope_node = self.inject_node(NodeID::jump_to());
globals
.add(JUMP_TO_SCOPE_NODE_VAR.into(), jump_to_scope_node.into())
.expect("Failed to set JUMP_TO_SCOPE_NODE");
if globals.get(&FILE_PATH_VAR.into()).is_none() {
let file_name = self.stack_graph[self.file].to_string();
globals
.add(FILE_PATH_VAR.into(), file_name.into())
.expect("Failed to set FILE_PATH");
}

let mut config = ExecutionConfig::new(&self.sgl.functions, &globals)
.lazy(true)
Expand Down
19 changes: 17 additions & 2 deletions tree-sitter-stack-graphs/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ use tree_sitter_loader::Loader as TsLoader;
use crate::CancellationFlag;
use crate::FileAnalyzer;
use crate::StackGraphLanguage;
use crate::FILE_PATH_VAR;

const BUILTINS_FILENAME: &str = "<builtins>";

pub static DEFAULT_TSG_PATHS: Lazy<Vec<LoadPath>> =
Lazy::new(|| vec![LoadPath::Grammar("queries/stack-graphs".into())]);
Expand Down Expand Up @@ -75,10 +78,15 @@ impl LanguageConfiguration {
let mut builtins = StackGraph::new();
if let Some((builtins_path, builtins_source)) = builtins_source {
let mut builtins_globals = Variables::new();

builtins_globals
.add(FILE_PATH_VAR.into(), BUILTINS_FILENAME.into())
.expect("failed to add file path variable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: move below the load globals and make optional so the config can override it.


if let Some(builtins_config) = builtins_config {
Loader::load_globals_from_config_str(builtins_config, &mut builtins_globals)?;
}
let file = builtins.add_file("<builtins>").unwrap();
let file = builtins.add_file(BUILTINS_FILENAME).unwrap();
sgl.build_stack_graph_into(
&mut builtins,
file,
Expand Down Expand Up @@ -325,8 +333,15 @@ impl Loader {
graph: &mut StackGraph,
cancellation_flag: &dyn CancellationFlag,
) -> Result<(), LoadError<'a>> {
let file = graph.add_file(&path.to_string_lossy()).unwrap();
let file_name = path.to_string_lossy();
let file: stack_graphs::arena::Handle<stack_graphs::graph::File> =
graph.add_file(&file_name).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the type annotation is necessary here, that should be possible to infer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes my bad, probably miss-clicked on the annotation in my IDE

let mut globals = Variables::new();

globals
.add(FILE_PATH_VAR.into(), BUILTINS_FILENAME.into())
.expect("failed to add file path variable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move after the load globals below, and make conditional, so the config can override the value.

Copy link
Contributor Author

@nohehf nohehf Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting it before was intended, load_globals_from_config_str will override the default value if any, no need for a conditional I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay, overriding with add will return an error if it already exists. I believe just overriding could be nice but will do

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Yes, if the load functions would add or set, that would also work.


Self::load_globals_from_config_str(&config, &mut globals)?;
sgl.build_stack_graph_into(graph, file, &source, &globals, cancellation_flag)
.map_err(|err| LoadError::Builtins {
Expand Down
20 changes: 16 additions & 4 deletions tree-sitter-stack-graphs/tests/it/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
// Licensed under either of Apache License, Version 2.0, or MIT license, at your option.
// Please see the LICENSE-APACHE or LICENSE-MIT files in this distribution for license details.
// ------------------------------------------------------------------------------------------------

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this.

use stack_graphs::graph::StackGraph;
use tree_sitter_graph::Variables;
use tree_sitter_stack_graphs::NoCancellation;
use tree_sitter_stack_graphs::StackGraphLanguage;
use tree_sitter_stack_graphs::FILE_PATH_VAR;

use crate::edges::check_stack_graph_edges;
use crate::nodes::check_stack_graph_nodes;
Expand All @@ -22,12 +22,18 @@ fn can_support_preexisting_nodes() {
"#;
let python = "pass";

let file_name = "test.py";

let mut graph = StackGraph::new();
let file = graph.get_or_create_file("test.py");
let file = graph.get_or_create_file(file_name);
let node_id = graph.new_node_id(file);
let _preexisting_node = graph.add_scope_node(node_id, true).unwrap();

let globals = Variables::new();
let mut globals = Variables::new();
globals
.add(FILE_PATH_VAR.into(), file_name.into())
.expect("failed to add file path variable");

let language = StackGraphLanguage::from_str(tree_sitter_python::language(), tsg).unwrap();
language
.build_stack_graph_into(&mut graph, file, python, &globals, &NoCancellation)
Expand All @@ -45,15 +51,21 @@ fn can_support_injected_nodes() {
"#;
let python = "pass";

let file_name = "test.py";

let mut graph = StackGraph::new();
let file = graph.get_or_create_file("test.py");
let file = graph.get_or_create_file(file_name);
let node_id = graph.new_node_id(file);
let _preexisting_node = graph.add_scope_node(node_id, true).unwrap();

let language = StackGraphLanguage::from_str(tree_sitter_python::language(), tsg).unwrap();
let mut builder = language.builder_into_stack_graph(&mut graph, file, python);

let mut globals = Variables::new();
globals
.add(FILE_PATH_VAR.into(), file_name.into())
.expect("failed to add file path variable");

globals
.add("EXT_NODE".into(), builder.inject_node(node_id).into())
.expect("Failed to add EXT_NODE variable");
Expand Down
14 changes: 12 additions & 2 deletions tree-sitter-stack-graphs/tests/it/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@
// Please see the LICENSE-APACHE or LICENSE-MIT files in this distribution for license details.
// ------------------------------------------------------------------------------------------------

use std::path::Path;

use stack_graphs::arena::Handle;
use stack_graphs::graph::File;
use stack_graphs::graph::StackGraph;
use tree_sitter_graph::Variables;
use tree_sitter_stack_graphs::BuildError;
use tree_sitter_stack_graphs::NoCancellation;
use tree_sitter_stack_graphs::StackGraphLanguage;
use tree_sitter_stack_graphs::FILE_PATH_VAR;

mod builder;
mod edges;
Expand All @@ -23,11 +26,18 @@ pub(self) fn build_stack_graph(
python_source: &str,
tsg_source: &str,
) -> Result<(StackGraph, Handle<File>), BuildError> {
let file_name = "test.py";
let language =
StackGraphLanguage::from_str(tree_sitter_python::language(), tsg_source).unwrap();
let mut graph = StackGraph::new();
let file = graph.get_or_create_file("test.py");
let globals = Variables::new();
let file = graph.get_or_create_file(file_name);
let mut globals = Variables::new();
let source_path = Path::new(file_name);

globals
.add(FILE_PATH_VAR.into(), source_path.to_str().unwrap().into())
.expect("failed to add file path variable");

language.build_stack_graph_into(&mut graph, file, python_source, &globals, &NoCancellation)?;
Ok((graph, file))
}
10 changes: 10 additions & 0 deletions tree-sitter-stack-graphs/tests/it/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// Please see the LICENSE-APACHE or LICENSE-MIT files in this distribution for license details.
// ------------------------------------------------------------------------------------------------

use crate::FILE_PATH_VAR;
use once_cell::sync::Lazy;
use pretty_assertions::assert_eq;
use stack_graphs::arena::Handle;
Expand Down Expand Up @@ -94,9 +95,18 @@ fn check_test(
expected_successes + expected_failures,
assertion_count,
);

let mut globals = Variables::new();
for fragments in &test.fragments {
globals.clear();

globals
.add(
FILE_PATH_VAR.into(),
fragments.path.to_str().unwrap().into(),
)
.expect("failed to add file path variable");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI failure is probably because this is still before loading the globals from the test, which can set file path too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe adding a add_if_not_set (not sure about the naming) convenience function to Variables would be a nice addition. Let me fix this


fragments.add_globals_to(&mut globals);
build_stack_graph_into(
&mut test.graph,
Expand Down
Loading