-
Notifications
You must be signed in to change notification settings - Fork 136
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
fix: python imports #438
Changes from 4 commits
7621ac2
b326053
8a98dce
ad4d517
5ba472a
ba1d3cd
a297647
6bcb5a3
c7f9fd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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())]); | ||
|
@@ -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"); | ||
|
||
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, | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting it before was intended, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
// ------------------------------------------------------------------------------------------------ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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) | ||
|
@@ -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"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe adding a |
||
|
||
fragments.add_globals_to(&mut globals); | ||
build_stack_graph_into( | ||
&mut test.graph, | ||
|
There was a problem hiding this comment.
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.