Skip to content

Commit 66af1d6

Browse files
Catherine Gasniermeta-codesync[bot]
authored andcommitted
Don't give reference targets in Buck outputs a Glean file
Summary: **Problem.** Pyrefly's Glean report (`--report-glean`) records, for each reference and import in an indexed file, the target's name and the file that defines it. That file is the target module's path relative to the working directory. When Pyrefly resolves imports through Buck (e.g. with `fbcode/pyrefly.toml`), generated modules such as Thrift Python packages resolve to build outputs, so the facts name files like `buck-out/.pyrelsp/art/fbcode/<path>/__<target>-python-<x>.thrift__/<hash>/out/gen-python/<pkg>`. In a test of `fbsource.genai.python.pyrefly` with Buck resolution, 285 such paths ended up in `src.File`. They contain a configuration hash, they change from build to build, and Glean never indexes those files, so they are noise in the DB. The Pyre-based Glean DB `fbsource.fbcode.python`, which the Pyrefly DB `fbsource.fbcode.python.pyrefly` is replacing (T291925475), has no `buck-out` paths: it only writes facts for the source files it indexes, and references reach generated code by name. **Change.** When the module that defines a reference target is not a source file of the repository (it is under `buck-out/`, or outside the working directory), the reference's definition location now has no file. This state already exists: `DefinitionLocation.file` is an `Option` and is `None` when Pyrefly can't find a definition. The target name is kept, so name-based references (`python.XRefViaName`) are unchanged. The indexed file's own `src.File` is unaffected. Review: `definition_file_fact` in `pyrefly/lib/report/glean/convert.rs`, its two call sites (`get_definition_location`, `find_definition_for_imported_module`), then the tests. ___ landed-with-radar-review Differential Revision: D125429038 fbshipit-source-id: 6e69c873cf2ad3930a3b0b87eab9f7d75cd2149a
1 parent 96c352b commit 66af1d6

1 file changed

Lines changed: 49 additions & 2 deletions

File tree

‎pyrefly/lib/report/glean/convert.rs‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ use std::collections::HashSet;
1010
use std::collections::VecDeque;
1111
use std::env::current_dir;
1212
use std::ops::Sub;
13+
use std::path::Path;
1314
use std::slice;
1415
use std::sync::Arc;
1516

@@ -165,6 +166,16 @@ fn file_fact(module_info: &ModuleInfo) -> src::File {
165166
src::File::new(relative_path.replace('\\', "/"))
166167
}
167168

169+
/// The file fact of the module that defines a cross-module reference target, or `None` if that
170+
/// module is not a source file of the repository: a Buck build output under `buck-out/` (e.g. a
171+
/// generated Thrift module) or a file outside the working directory. Glean only indexes source
172+
/// files, so such a file would never have facts of its own. The target name is kept either way.
173+
fn definition_file_fact(module_info: &ModuleInfo) -> Option<src::File> {
174+
let file = file_fact(module_info);
175+
let is_source_file = !file.key.starts_with("buck-out/") && !Path::new(&*file.key).is_absolute();
176+
is_source_file.then_some(file)
177+
}
178+
168179
fn gather_nonlocal_variables(body: &[Stmt]) -> (Arc<SmallSet<Name>>, Arc<SmallSet<Name>>) {
169180
let mut globals = SmallSet::new();
170181
let mut nonlocals = SmallSet::new();
@@ -525,7 +536,7 @@ impl GleanState<'_> {
525536
base_type: Option<&Type>,
526537
additional_definitions: Vec<DefinitionLocation>,
527538
) -> Vec<DefinitionLocation> {
528-
let file = Some(file_fact(module));
539+
let file = definition_file_fact(module);
529540
let type_name = base_type.and_then(|ty| ty.qname());
530541
let local_name = module.code_at(def_range);
531542
let module_name = module.name();
@@ -1236,7 +1247,7 @@ impl GleanState<'_> {
12361247
.unwrap_or(None);
12371248

12381249
let name = module.to_string();
1239-
let file = definition.map(|def| file_fact(&def.module));
1250+
let file = definition.and_then(|def| definition_file_fact(&def.module));
12401251

12411252
DefinitionLocation { name, file }
12421253
}
@@ -1761,8 +1772,44 @@ pub fn glean(transaction: &Transaction, handle: &Handle, ownership: bool) -> Gle
17611772

17621773
#[cfg(test)]
17631774
mod tests {
1775+
use std::path::PathBuf;
1776+
1777+
use pyrefly_python::module_path::ModulePath;
1778+
17641779
use super::*;
17651780

1781+
fn module_at(path: &str) -> ModuleInfo {
1782+
ModuleInfo::new(
1783+
ModuleName::from_str("m"),
1784+
ModulePath::filesystem(PathBuf::from(path)),
1785+
Arc::new(String::new()),
1786+
)
1787+
}
1788+
1789+
#[test]
1790+
fn test_definition_file_fact_source_file() {
1791+
let file = definition_file_fact(&module_at("fbcode/foo/bar.py"));
1792+
assert_eq!(file.map(|f| *f.key), Some("fbcode/foo/bar.py".to_owned()));
1793+
}
1794+
1795+
#[test]
1796+
fn test_definition_file_fact_buck_out() {
1797+
let path = "buck-out/.pyrelsp/art/fbcode/foo/__bar-python-bar.thrift__/out/gen-python/foo";
1798+
assert!(
1799+
definition_file_fact(&module_at(path)).is_none(),
1800+
"Buck outputs have no file fact"
1801+
);
1802+
}
1803+
1804+
#[test]
1805+
fn test_definition_file_fact_outside_working_directory() {
1806+
let file = definition_file_fact(&module_at("/nonexistent/site-packages/foo.py"));
1807+
assert!(
1808+
file.is_none(),
1809+
"files outside the working directory have no file fact"
1810+
);
1811+
}
1812+
17661813
#[test]
17671814
fn test_compute_scope_module() {
17681815
assert_eq!(

0 commit comments

Comments
 (0)