diff --git a/README.md b/README.md index 12447a48..f6106679 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,8 @@ Lint rules are in groups. The `LINT` rules are in the `default` group, which is |---|---|---|---| | CL001 | `run-value` | `strict.calls` | `RUN VALUE(...)` with a computed target | | CL002 | `dynamic-invoke` | `strict.calls` | `DYNAMIC-FUNCTION`, `DYNAMIC-INVOKE`, `DYNAMIC-NEW`, `DYNAMIC-PROPERTY`, `CREATE CALL` | +| CL003 | `signature-mismatch` | `strict.calls` | a static `RUN` whose arguments differ from the callee's parameters in count, kind (`TABLE`, `BUFFER`, ...), mode (`INPUT`, `OUTPUT`, `INPUT-OUTPUT`) or data type; an internal procedure, a program on the search paths, or a procedure `IN h` of a program the file runs `PERSISTENT SET h` | +| CL004 | `unresolved-call` | `strict.calls` | a static `RUN` of a name that is neither a procedure of the file nor a program on the search paths; `LINT0001` reports the same miss for a name with an extension, so a `RUN` is never reported twice | | CL005 | `shared-state` | `strict.calls` | `NEW SHARED`, `NEW GLOBAL SHARED` and `SHARED` definitions | | TX006 | `missing-no-undo` | `strict.transactions` | a variable, parameter, temp-table or property without `NO-UNDO` | | HD001 | `handle-not-deleted` | `strict.handles` | a dynamic object (query, buffer, temp-table, dataset, widget, ...) that some path loses without a delete | diff --git a/crates/oxabl/examples/calls_corpus.rs b/crates/oxabl/examples/calls_corpus.rs new file mode 100644 index 00000000..5c8f33aa --- /dev/null +++ b/crates/oxabl/examples/calls_corpus.rs @@ -0,0 +1,214 @@ +//! calls_corpus — run `signature-mismatch` (CL003) and `unresolved-call` +//! (CL004) over a tree the way `oxabl check ` does, and print the +//! finding counts per rule per top-level directory, with the wall time. +//! +//! The tree's `oxabl.toml` is read as `check` reads it (its schema and search +//! paths included); the two rules are switched on in memory. Roots go through +//! `LintPipeline::run`, and the `.i` files of the tree through +//! `run_context_free`. `OXABL_CALLS_GROUP=off` leaves the rules as configured, +//! to time the same run without them. +//! +//! The CL003 findings are also counted by kind: argument count, kind (`TABLE`, +//! `BUFFER`, ...), mode, an expression passed as `OUTPUT`, and data type. +//! +//! # Usage +//! ```text +//! OXABL_CALLS_CORPUS= cargo run -p oxabl --release --example calls_corpus +//! ``` +//! `OXABL_CALLS_ONLY=` checks only the files whose path holds the text. +//! `OXABL_CALLS_FINDINGS=` also writes every finding as a tab-separated +//! line: rule, `path:line`, the message and the help. + +use std::collections::{BTreeMap, HashSet}; +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::time::Instant; + +use oxabl_analyze::DiagnosticSource; +use oxabl_pipeline::{ConfigOverrides, LintPipeline, PipelineConfig}; +use oxabl_workspace::{RealFileSystem, discover_path, walk_include_fragments}; + +const RULES: [&str; 2] = ["CL003", "CL004"]; +const N: usize = RULES.len(); + +struct Finding { + rule: &'static str, + path: PathBuf, + line: usize, + use_site: bool, + message: String, + help: String, +} + +fn main() { + let Some(root) = std::env::var_os("OXABL_CALLS_CORPUS").map(PathBuf::from) else { + eprintln!("set OXABL_CALLS_CORPUS to a directory"); + std::process::exit(2); + }; + let started = Instant::now(); + let (mut config, warnings) = PipelineConfig::resolve(&root, &ConfigOverrides::default()); + for w in &warnings { + eprintln!("warning: {w}"); + } + if std::env::var("OXABL_CALLS_GROUP").is_ok_and(|v| v == "off") { + eprintln!("the call rules are left as configured"); + } else { + for rule in RULES { + config + .lint_severities + .base_mut() + .set_rule(rule, Some(oxabl_common::Severity::Warning)); + } + } + + let mut roots = discover_path(&root).unwrap_or_else(|e| { + eprintln!("error: {e}"); + std::process::exit(2); + }); + // Narrow the run to the files whose path holds the text, to look at one. + let only = std::env::var("OXABL_CALLS_ONLY").ok(); + if let Some(only) = &only { + roots.retain(|f| f.to_string_lossy().contains(only.as_str())); + } + let abs = |p: &Path| std::path::absolute(p).unwrap_or_else(|_| p.to_path_buf()); + let indexed: Vec = roots.iter().map(|f| abs(f)).collect(); + let fs = RealFileSystem; + let lint = LintPipeline::new(&config, &fs) + .with_preprocess(true) + .with_known_files(&indexed); + let fragments: Vec = walk_include_fragments(&root) + .into_iter() + .filter(|f| { + only.as_ref() + .is_none_or(|only| f.to_string_lossy().contains(only.as_str())) + }) + .filter(|f| lint.with_file(abs(f)).context_free_rules_enabled()) + .collect(); + + let mut findings: Vec = Vec::new(); + let mut unreadable = 0usize; + let mut panics: Vec = Vec::new(); + let mut seen: HashSet<(PathBuf, &'static str, u32)> = HashSet::new(); + for (file, is_fragment) in roots + .iter() + .map(|f| (f, false)) + .chain(fragments.iter().map(|f| (f, true))) + { + let Ok(bytes) = std::fs::read(file) else { + unreadable += 1; + continue; + }; + let source = String::from_utf8_lossy(&bytes).into_owned(); + let handle = lint.with_file(abs(file)); + let result = if is_fragment { + handle.run_context_free(&source) + } else { + handle.run(&source) + }; + if let Some(panic) = result.failure() { + panics.push(format!("{}: {panic:?}", file.display())); + continue; + } + let starts: Vec = std::iter::once(0) + .chain(source.match_indices('\n').map(|(i, _)| i + 1)) + .collect(); + for found in result.by_source(DiagnosticSource::Lint) { + let d = &found.diagnostic; + let Some(rule) = RULES.iter().find(|r| **r == d.code.0) else { + continue; + }; + if !seen.insert((file.clone(), rule, d.span.span.start)) { + continue; + } + let help = d.help.as_deref().unwrap_or(""); + findings.push(Finding { + rule, + path: file.clone(), + line: starts.partition_point(|&s| s <= d.span.span.start as usize), + use_site: help.ends_with(oxabl_lint::fragment::USE_SITE_NOTE), + message: d.message.clone(), + help: help.to_string(), + }); + } + } + let wall = started.elapsed(); + + let top = |p: &Path| -> String { + let rel = p.strip_prefix(&root).unwrap_or(p); + let mut parts = rel.components(); + match (parts.next(), parts.next()) { + (Some(first), Some(_)) => first.as_os_str().to_string_lossy().into_owned(), + _ => ".".to_string(), + } + }; + let mut table: BTreeMap = BTreeMap::new(); + let mut totals = [0usize; N]; + let mut use_site = [0usize; N]; + for f in &findings { + let i = RULES.iter().position(|r| *r == f.rule).unwrap(); + table.entry(top(&f.path)).or_default()[i] += 1; + totals[i] += 1; + use_site[i] += usize::from(f.use_site); + } + println!("root {}", root.display()); + println!("roots checked {}", roots.len()); + println!("include files checked {}", fragments.len()); + println!("unreadable {unreadable}"); + println!("panics {}", panics.len()); + for p in panics.iter().take(10) { + println!(" {p}"); + } + println!(); + let row = |name: &str, c: &[usize; N]| { + let cells: String = c.iter().map(|n| format!("{n:>7}")).collect(); + println!("{name:<24}{cells}{:>9}", c.iter().sum::()); + }; + let head: String = RULES.iter().map(|r| format!("{r:>7}")).collect(); + println!("{:<24}{head}{:>9}", "directory", "total"); + for (dir, counts) in &table { + row(dir, counts); + } + row("TOTAL", &totals); + row("at a use site", &use_site); + println!("wall time {wall:.2?}"); + let mut kinds: BTreeMap<&str, usize> = BTreeMap::new(); + for f in findings.iter().filter(|f| f.rule == "CL003") { + *kinds.entry(kind_of(&f.message)).or_default() += 1; + } + for (kind, n) in &kinds { + println!("CL003 {kind:<22}{n:>7}"); + } + + if let Some(out) = std::env::var_os("OXABL_CALLS_FINDINGS") { + findings.sort_by(|a, b| (a.rule, &a.path, a.line).cmp(&(b.rule, &b.path, b.line))); + let mut w = std::io::BufWriter::new(std::fs::File::create(out).expect("create findings")); + for f in &findings { + writeln!( + w, + "{}\t{}:{}\t{}\t{}\t{}", + f.rule, + f.path.display(), + f.line, + if f.use_site { "use-site" } else { "-" }, + f.message, + f.help + ) + .expect("write findings"); + } + } +} + +/// What a CL003 message reports. +fn kind_of(message: &str) -> &'static str { + if message.contains("but the procedure takes") { + "count" + } else if message.contains("is an expression") { + "expression as output" + } else if message.contains("passed as") { + "mode" + } else if message.contains("but the parameter is a `") { + "kind" + } else { + "type" + } +} diff --git a/crates/oxabl_daemon/src/db.rs b/crates/oxabl_daemon/src/db.rs index 2aa2b37b..c47d19ea 100644 --- a/crates/oxabl_daemon/src/db.rs +++ b/crates/oxabl_daemon/src/db.rs @@ -102,7 +102,7 @@ use std::sync::{Arc, RwLock}; use oxabl_analyze::CollectedDiagnostics; use oxabl_index::search::{self, NameKind}; -use oxabl_index::{FileFacts, IncludeContext, index_file_at}; +use oxabl_index::{FileFacts, IncludeContext, index_file_at, program_signatures_at}; use oxabl_pipeline::{Expansion, LintPipeline, PipelineConfig}; use oxabl_semantic::{ ClassDescriptor, IndexAnswer, IndexName, IndexRevision, IndexedFileId, LocateKind, Located, @@ -684,6 +684,38 @@ fn indexed_class_members( } } +/// The parameter lists of one file, memoized per file and read on the first +/// question about them: a buffer whose rules ask none never reads the file for +/// them. +/// +/// Reads the file and its includes the way [`indexed_facts`] does, and registers +/// each include's revision, so an edit to the file or to an include re-reads the +/// lists. An equal result backdates, so a body edit that leaves the parameters +/// alone stops here. +#[salsa::tracked(returns(clone))] +fn indexed_signatures( + db: &dyn AblDatabase, + file: IndexedFile, +) -> Option> { + let _revision = file.disk_revision(db); + let environment = db.config().index.environment(db); + let path = file.path(db); + let source = db.config().fs.read(path).ok()?; + db.unwind_if_revision_cancelled(); + let config = db.config(); + let includes = IncludeContext { + fs: &*config.fs, + include_paths: environment.include_paths(db), + root_relative_paths: environment.root_relative_include_paths(db), + predefined: environment.preprocessor(db), + }; + let (signatures, read) = program_signatures_at(path, &source, &includes); + for path in read { + let _revision = config.index.include_for(db, &path).disk_revision(db); + } + signatures.map(Arc::new) +} + /// Whether a located file is usable as a `RUN` target, keyed per file. /// /// A broken file answers `Unusable` — located, and oxabl could not read it, which @@ -841,6 +873,22 @@ impl WorkspaceIndex for SnapshotIndex<'_> { } } + fn signature( + &self, + program: &IndexName, + procedure: Option<&IndexName>, + ) -> Option> { + let IndexAnswer::Found(file) = self.locate(program, NameKind::Program) else { + return None; + }; + let signatures = indexed_signatures(self.db, file)?; + let list = match procedure { + Some(name) => signatures.procedure(name), + None => signatures.main.as_ref(), + }; + list.map(|s| Arc::new(s.clone())) + } + fn located(&self, kind: LocateKind, name: &IndexName) -> Located { let config = self.db.config(); let kind = match kind { @@ -1862,6 +1910,62 @@ mod tests { assert_eq!(tx004(&db, buffer, schema), 0); } + fn calls_config(fs: Arc) -> AnalysisConfig { + let mut config = searching_config(fs); + Arc::make_mut(&mut config.pipeline) + .lint_severities + .base_mut() + .enable_group("strict.calls"); + config + } + + fn cl003(db: &AnalysisDatabase, buffer: Buffer, schema: SchemaHandle) -> usize { + compute_diagnostics(db, buffer, schema) + .unwrap() + .all() + .filter(|d| d.diagnostic.code.0 == "CL003") + .count() + } + + #[test] + fn parameter_lists_follow_edits_to_the_callee_and_its_includes() { + let fs = EditableFs::new(&[ + ( + "/src/callee.p", + "DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.", + ), + ("/src/other.p", "{head.i}\nMESSAGE 1."), + ( + "/src/head.i", + "DEFINE OUTPUT PARAMETER b AS INTEGER NO-UNDO.", + ), + ]); + let mut db = AnalysisDatabase::new(calls_config(Arc::clone(&fs) as Arc)); + let buffer = Buffer::new( + &db, + "RUN callee.p (INPUT 1, INPUT 2).\nRUN other.p (INPUT 1).".into(), + Some("/src/main.p".into()), + ); + let schema = SchemaHandle::new(&db, 0); + // The count differs for `callee.p`; the mode differs for `other.p`, whose + // parameter an include defines. + assert_eq!(cl003(&db, buffer, schema), 2); + + fs.write( + "/src/callee.p", + "DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\nDEFINE INPUT PARAMETER c AS INTEGER NO-UNDO.", + ); + assert!(bump(&mut db, "/src/callee.p")); + assert_eq!(cl003(&db, buffer, schema), 1); + + fs.write( + "/src/head.i", + "DEFINE INPUT PARAMETER b AS INTEGER NO-UNDO.", + ); + assert!(bump(&mut db, "/src/head.i")); + assert_eq!(cl003(&db, buffer, schema), 0); + } + #[test] fn summaries_invalidate_includes_and_search_misses() { let fs = EditableFs::new(&[("/src/a.p", "{body.i}"), ("/src/body.i", "RUN missing.p.")]); diff --git a/crates/oxabl_index/src/batch.rs b/crates/oxabl_index/src/batch.rs index 91e09056..9506bc96 100644 --- a/crates/oxabl_index/src/batch.rs +++ b/crates/oxabl_index/src/batch.rs @@ -17,14 +17,14 @@ use std::sync::{Arc, Mutex, RwLock, RwLockWriteGuard}; use oxabl_lexer::oxabl_atom::OxablAtom; use oxabl_semantic::{ ClassDescriptor, IndexAnswer, IndexName, IndexRevision, IndexedFileId, LocateKind, Located, - MemberDescriptor, WorkspaceIndex, + MemberDescriptor, ProgramSignatures, Signature, WorkspaceIndex, }; use oxabl_workspace::{FileSystem, PreprocessorConfig}; use rustc_hash::{FxHashMap, FxHashSet}; use crate::facts::{ClassFacts, FileFacts}; -use crate::producers::{SharedProducers, may_define_shared}; -use crate::{IncludeContext, index_file_at, search}; +use crate::producers::{SharedProducers, may_define_shared, scan_procedures}; +use crate::{IncludeContext, index_file_at, program_signatures_at, search}; /// The generation a batch reports. /// @@ -131,6 +131,12 @@ struct Memo { scoped_classes: FxHashMap<(PathBuf, OxablAtom), IndexAnswer>, /// Answers per asking file's directory and `RUN` target key. scoped_programs: FxHashMap<(PathBuf, OxablAtom), IndexAnswer>, + /// Parameter lists per indexed file, read on the first question about them: + /// a run that asks none reads nothing. + signatures: FxHashMap>>, + /// The internal procedures the files of [`ProducerSource::Files`] define, + /// read on the first question about them. + procedure_names: Option>>>, /// The inverse of `facts`: the path each minted id was indexed under. /// /// Kept rather than derived. Deriving it means scanning `facts` for the entry @@ -185,6 +191,8 @@ impl<'a> BatchIndex<'a> { programs: FxHashMap::default(), scoped_classes: FxHashMap::default(), scoped_programs: FxHashMap::default(), + signatures: FxHashMap::default(), + procedure_names: None, next_file_id: 1, known_indexed: false, seeded: FxHashSet::default(), @@ -455,6 +463,50 @@ impl<'a> BatchIndex<'a> { /// lookup has answered: by then the file behind the answer is in `facts`, so /// [`ExcludingFile`] can compare ids without having to know how the search /// turned a name into a path. + /// The parameter lists of the indexed file `file`, or `None` when the file + /// cannot be read or parsed. + /// + /// A list the file's own text does not show whole (an `{include}` may hold + /// a parameter) is read again from the expanded text. + fn signatures_of(&self, file: IndexedFileId) -> Option> { + let path = { + let memo = self.memo(); + if let Some(hit) = memo.signatures.get(&file) { + return hit.clone(); + } + memo.paths.get(&file).cloned()? + }; + let answer = self.read_signatures(&path).map(Arc::new); + self.memo().signatures.insert(file, answer.clone()); + answer + } + + fn read_signatures(&self, path: &Path) -> Option { + let source = self.fs.read(path).ok()?; + let predefined = self.predefined(); + program_signatures_at(path, &source, &self.includes(&predefined)).0 + } + + /// See [`WorkspaceIndex::workspace_defines_procedure`]. + fn defines_procedure(&self, name: &IndexName) -> Option { + let predefined = self.predefined(); + let includes = self.includes(&predefined); + match self.producers { + ProducerSource::None => None, + ProducerSource::Shared(map) => Some(map.defines_procedure(name, &includes)), + ProducerSource::Files(files) => { + let known = self.memo().procedure_names.clone(); + let names = known.unwrap_or_else(|| { + let threads = std::thread::available_parallelism().map_or(1, |n| n.get()); + let scanned = Arc::new(scan_procedures(files, &includes, threads)); + self.memo().procedure_names = Some(Arc::clone(&scanned)); + scanned + }); + Some(names.contains(name.as_str())) + } + } + } + fn indexed_id(&self, path: &Path) -> Option { let key = search::normalize_lexically(path); self.memo().facts.get(&key).map(|facts| facts.file) @@ -720,6 +772,21 @@ impl WorkspaceIndex for BatchIndex<'_> { self.shared_producer_among(name, None) } + fn workspace_defines_procedure(&self, name: &IndexName) -> Option { + self.defines_procedure(name) + } + + fn signature( + &self, + program: &IndexName, + procedure: Option<&IndexName>, + ) -> Option> { + let IndexAnswer::Found(file) = self.program(program) else { + return None; + }; + signature_in(&*self.signatures_of(file)?, procedure) + } + fn located(&self, kind: LocateKind, name: &IndexName) -> Located { let kind = match kind { LocateKind::Class => search::NameKind::Class, @@ -751,6 +818,18 @@ impl WorkspaceIndex for BatchIndex<'_> { } } +/// The list of the program or of its internal procedure `procedure`. +fn signature_in( + signatures: &ProgramSignatures, + procedure: Option<&IndexName>, +) -> Option> { + let list = match procedure { + Some(name) => signatures.procedure(name), + None => signatures.main.as_ref(), + }; + list.map(|s| Arc::new(s.clone())) +} + /// One asking file's view of a [`BatchIndex`]: every answer that would name the /// file being analysed becomes [`IndexAnswer::NotFound`]. /// @@ -850,6 +929,24 @@ impl WorkspaceIndex for ExcludingFile<'_, '_> { } } + fn workspace_defines_procedure(&self, name: &IndexName) -> Option { + self.index.defines_procedure(name) + } + + fn signature( + &self, + program: &IndexName, + procedure: Option<&IndexName>, + ) -> Option> { + let IndexAnswer::Found(file) = self.index.program_from(program, self.path) else { + return None; + }; + if self.is_analysed(file) { + return None; + } + signature_in(&*self.index.signatures_of(file)?, procedure) + } + fn shared_producer(&self, name: &IndexName) -> IndexAnswer { let reached = self .reached diff --git a/crates/oxabl_index/src/lib.rs b/crates/oxabl_index/src/lib.rs index 6e3dcfb2..6317c7f6 100644 --- a/crates/oxabl_index/src/lib.rs +++ b/crates/oxabl_index/src/lib.rs @@ -92,7 +92,8 @@ use oxabl_preprocessor::{PreprocessedFile, Preprocessor, SpanNode}; use oxabl_schema::Schema; use oxabl_semantic::{ AnalysisContext, ClassDescriptor, ClassKind, ImportContext, IndexName, IndexedFileId, - MemberDescriptor, MemberType, ResolvedType, ScopeId, SymbolFlags, SymbolKind, declare_pass, + MemberDescriptor, MemberType, ProgramSignatures, ResolvedType, ScopeId, SymbolFlags, + SymbolKind, declare_pass, }; use oxabl_workspace::{FileSystem, PreprocessorConfig}; @@ -159,6 +160,47 @@ pub fn index_file_at( index_file_with_includes(file, source, &includes) } +/// The parameter lists of the file at `path`, and the include files they were +/// read through (as [`index_file_with_includes`] reports them). +/// +/// A list the text as written does not show whole (an `{include}` may hold a +/// parameter) is read again from the expanded text; when that does not settle +/// either, the list stays unknown. `None` when the file does not parse. +pub fn program_signatures_at( + path: &Path, + source: &str, + includes: &IncludeContext<'_>, +) -> (Option, Vec) { + fn parse(source: &str) -> Option { + let tokens = oxabl_lexer::tokenize(source); + let program = Parser::new(&tokens, source).parse_program(); + program + .is_ok() + .then(|| ProgramSignatures::of(&program.statements, &tokens)) + } + let Some(written) = parse(source) else { + return (None, Vec::new()); + }; + if written.main.is_some() && written.procedures.iter().all(|(_, s)| s.is_some()) { + return (Some(written), Vec::new()); + } + let paths = includes.paths_for(path); + let includes = IncludeContext { + include_paths: &paths, + root_relative_paths: &[], + ..*includes + }; + let Some(expansion) = expand(source, &includes) else { + return (Some(written), Vec::new()); + }; + let read = included_paths(&expansion.processed); + let signatures = expansion + .text + .and_then(|text| parse(&text)) + .map(|expanded| expanded.marking_includes(&written)); + (Some(signatures.unwrap_or(written)), read) +} + /// A filesystem with nothing in it. struct NoFiles; diff --git a/crates/oxabl_index/src/producers.rs b/crates/oxabl_index/src/producers.rs index 417b742b..e481c106 100644 --- a/crates/oxabl_index/src/producers.rs +++ b/crates/oxabl_index/src/producers.rs @@ -22,10 +22,12 @@ //! missing, on every thread and in every order alike. use std::path::{Path, PathBuf}; +use std::sync::OnceLock; use std::sync::atomic::{AtomicUsize, Ordering}; +use oxabl_lexer::Kind; use oxabl_semantic::IndexName; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use crate::{IncludeContext, IndexedFileId, index_file_at}; @@ -39,8 +41,31 @@ use crate::{IncludeContext, IndexedFileId, index_file_at}; #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct SharedProducers { by_name: FxHashMap, Vec>, + /// The files the map was built over, in path order. + files: Vec, + procedures: ProcedureNames, } +/// The names of the internal procedures the files of a [`SharedProducers`] +/// define, read on the first question and not before. Two maps are equal +/// whatever either has read. +#[derive(Debug, Default)] +struct ProcedureNames(OnceLock>>); + +impl Clone for ProcedureNames { + fn clone(&self) -> Self { + ProcedureNames::default() + } +} + +impl PartialEq for ProcedureNames { + fn eq(&self, _: &Self) -> bool { + true + } +} + +impl Eq for ProcedureNames {} + impl SharedProducers { /// Scan `files` and record which of them produce each `SHARED` name. /// @@ -96,7 +121,9 @@ impl SharedProducers { /// the order of `files`. pub fn from_definitions(files: impl IntoIterator)>) -> Self { let mut by_name: FxHashMap, Vec> = FxHashMap::default(); + let mut listed = Vec::new(); for (path, names) in files { + listed.push(path.clone()); for name in names { let paths = by_name.entry(name.as_str().into()).or_default(); if !paths.contains(&path) { @@ -107,7 +134,30 @@ impl SharedProducers { for paths in by_name.values_mut() { paths.sort(); } - SharedProducers { by_name } + listed.sort(); + SharedProducers { + by_name, + files: listed, + procedures: ProcedureNames::default(), + } + } + + /// Whether any file of the map defines an internal procedure called + /// `name`, as the text of the file is written. + /// + /// The files are read on the first question, on every available thread; + /// a map that is never asked reads nothing. The include files the files + /// name are read too (see [`scan_procedures`]). A name in a file that does + /// not read, and one that only an include named by `{&name}` or `{1}` + /// supplies, is not seen. + pub fn defines_procedure(&self, name: &IndexName, includes: &IncludeContext<'_>) -> bool { + self.procedures + .0 + .get_or_init(|| { + let threads = std::thread::available_parallelism().map_or(1, |n| n.get()); + scan_procedures(&self.files, includes, threads) + }) + .contains(name.as_str()) } /// The files that define `name`, in path order. @@ -148,6 +198,150 @@ pub fn definitions_in(path: &Path, includes: &IncludeContext<'_>) -> Vec, + threads: usize, +) -> FxHashSet> { + let threads = threads.clamp(1, files.len().max(1)); + let next = AtomicUsize::new(0); + let scan = || { + let mut mine = Scanned::default(); + while let Some(path) = files.get(next.fetch_add(1, Ordering::Relaxed)) { + if let Ok(source) = includes.fs.read(path) { + mine.add(path.parent(), &source); + } + } + mine + }; + let mut all = if threads == 1 { + scan() + } else { + std::thread::scope(|scope| { + let workers: Vec<_> = (0..threads).map(|_| scope.spawn(scan)).collect(); + let mut all = Scanned::default(); + for worker in workers { + match worker.join() { + Ok(mine) => all.merge(mine), + Err(panic) => std::panic::resume_unwind(panic), + } + } + all + }) + }; + // The include files, each read once, and the ones they include. + let mut seen: FxHashSet = FxHashSet::default(); + let mut pending: Vec<(Option, String)> = all.includes.drain().collect(); + pending.sort(); + while let Some((dir, name)) = pending.pop() { + let own: Vec = dir.into_iter().collect(); + let Some(path) = includes + .fs + .resolve_include(&own, &name) + .or_else(|| includes.fs.resolve_include(includes.include_paths, &name)) + .or_else(|| { + includes + .fs + .resolve_include(includes.root_relative_paths, &name) + }) + else { + continue; + }; + if !seen.insert(path.clone()) { + continue; + } + if let Ok(source) = includes.fs.read(&path) { + let mut found = Scanned::default(); + found.add(path.parent(), &source); + all.names.extend(found.names); + pending.extend(found.includes); + } + } + all.names +} + +/// What a scan of source files found: procedure names, and the include +/// references to follow, each with the directory of the file that holds it. +#[derive(Default)] +struct Scanned { + names: FxHashSet>, + includes: FxHashSet<(Option, String)>, +} + +impl Scanned { + fn add(&mut self, dir: Option<&Path>, source: &str) { + procedure_names(source, &mut self.names); + for name in include_names(source) { + self.includes + .insert((dir.map(Path::to_path_buf), name.to_string())); + } + } + + fn merge(&mut self, other: Scanned) { + self.names.extend(other.names); + self.includes.extend(other.includes); + } +} + +/// The written-out names of the `{include}` references of `source`: the word +/// after a `{` that does not start a `{&name}`, a `{1}` or a `{}`. +fn include_names(source: &str) -> impl Iterator { + source.match_indices('{').filter_map(|(at, _)| { + let rest = &source[at + 1..]; + let first = rest.chars().next()?; + if !(first.is_alphanumeric() || matches!(first, '_' | '.' | '/' | '\\' | '"' | '\'')) + || first.is_ascii_digit() + { + return None; + } + let end = rest.find(|c: char| c.is_whitespace() || matches!(c, '}' | '{'))?; + let name = rest[..end].trim_matches(['"', '\'']); + (!name.is_empty() && !name.contains(['&', '*'])).then_some(name) + }) +} + +/// Add the folded name of every `PROCEDURE name` in `source` to `out`. A word +/// after `PROCEDURE` that is not a name is added too: `SUBSCRIBE PROCEDURE h` +/// names a handle. That only makes a question answer "defined" more often. +fn procedure_names(source: &str, out: &mut FxHashSet>) { + // The keyword may be abbreviated to `PROC`. + if !source + .as_bytes() + .windows(4) + .any(|w| w.eq_ignore_ascii_case(b"proc")) + { + return; + } + let tokens = oxabl_lexer::tokenize(source); + for pair in tokens.windows(2) { + // The lexer reads the abbreviations as plain words. + let keyword = pair[0].kind == Kind::Procedure + || (pair[0].kind == Kind::Identifier + && is_procedure_abbreviation(&source[pair[0].start..pair[0].end])); + if !keyword { + continue; + } + let word = source[pair[1].start..pair[1].end].trim_matches(['"', '\'']); + if word.starts_with(|c: char| c.is_alphanumeric() || c == '_') { + out.insert(IndexName::new(word).as_str().into()); + } + } +} + +/// `PROC`, `PROCE`, ... up to `PROCEDURE`, in any case. +fn is_procedure_abbreviation(word: &str) -> bool { + const FULL: &str = "procedure"; + (4..=FULL.len()).contains(&word.len()) && FULL[..word.len()].eq_ignore_ascii_case(word) +} + /// Whether [`index_file_at`] can find a shared definition in `source`, the file /// at `path`: a superset test. A file whose text does not say `SHARED` is parsed /// only when it names an include, and expanded only when `index_file_at` may @@ -358,6 +552,85 @@ mod tests { } } + #[test] + fn procedure_names_are_read_in_any_spelling() { + let files = [ + ( + "/a.p", + "PROCEDURE one-a:\nEND PROCEDURE.\nproc two-b:\nEND PROCEDURE.\nPROCEDURE \"Three-C\":\nEND.", + ), + ("/b.p", "MESSAGE \"proc nothing\".\nEND PROCEDURE."), + ]; + let fs = fs_with(&files); + let predefined = PreprocessorConfig::default(); + let includes = in_fs(&fs, &predefined); + let map = build(&fs, &paths(&files)); + for (name, want) in [ + ("one-a", true), + ("TWO-B", true), + ("three-c", true), + ("nothing", false), + ("missing", false), + ] { + assert_eq!( + map.defines_procedure(&IndexName::new(name), &includes), + want, + "{name}" + ); + } + // The answer does not depend on the number of threads. + let one = scan_procedures(&paths(&files), &includes, 1); + assert_eq!(scan_procedures(&paths(&files), &includes, 8), one); + assert_eq!(one.len(), 3); + } + + #[test] + fn the_include_files_a_file_names_are_followed() { + let files = [ + ("/src/super.p", "{lib/units.i}\n{&dynamic}\n{1}\n"), + ( + "/src/lib/units.i", + "PROCEDURE convert-units:\nEND PROCEDURE.\n{deeper.i}", + ), + ( + "/src/lib/deeper.i", + "PROCEDURE from-deeper:\nEND PROCEDURE.", + ), + ( + "/src/unused.i", + "PROCEDURE nobody-includes-me:\nEND PROCEDURE.", + ), + ]; + let fs = fs_with(&files); + let predefined = PreprocessorConfig::default(); + let include_paths = [PathBuf::from("/src")]; + let includes = IncludeContext { + fs: &fs, + include_paths: &include_paths, + root_relative_paths: &[], + predefined: &predefined, + }; + let names = scan_procedures(&paths(&files[..1]), &includes, 2); + assert!(names.contains("convert-units")); + // An include of an include is followed, from the including file's own + // directory. + assert!(names.contains("from-deeper")); + assert!(!names.contains("nobody-includes-me")); + } + + #[test] + fn a_map_reads_no_procedure_until_it_is_asked() { + let files = [("/a.p", "PROCEDURE x:\nEND PROCEDURE.")]; + let fs = fs_with(&files); + let map = build(&fs, &paths(&files)); + assert!(map.procedures.0.get().is_none()); + // Two maps are equal whatever either has read. + let other = build(&fs, &paths(&files)); + let predefined = PreprocessorConfig::default(); + other.defines_procedure(&IndexName::new("x"), &in_fs(&fs, &predefined)); + assert_eq!(map, other); + } + #[test] fn a_parallel_scan_builds_the_map_a_serial_scan_builds() { let sources: Vec<(String, String)> = (0..40) diff --git a/crates/oxabl_index/tests/signatures.rs b/crates/oxabl_index/tests/signatures.rs new file mode 100644 index 00000000..ede68d98 --- /dev/null +++ b/crates/oxabl_index/tests/signatures.rs @@ -0,0 +1,154 @@ +//! Parameter lists through the run index: what `WorkspaceIndex::signature` +//! answers for a program and for an internal procedure, and what it declines. + +use std::path::PathBuf; + +use oxabl_index::BatchIndex; +use oxabl_semantic::{IndexName, ParamMode, ParamShape, PrimitiveTy, Signature, WorkspaceIndex}; +use oxabl_workspace::InMemoryFileSystem; + +fn signature( + files: &[(&str, &str)], + program: &str, + procedure: Option<&str>, +) -> Option> { + let mut fs = InMemoryFileSystem::new(); + for (path, text) in files { + fs.insert(PathBuf::from(path), *text); + } + let paths = [PathBuf::from("/src")]; + let index = BatchIndex::new(&fs, &paths); + index.signature( + &IndexName::new(program), + procedure.map(IndexName::new).as_ref(), + ) +} + +const TWO: &str = "\ +DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. +DEFINE OUTPUT PARAMETER cText AS CHARACTER NO-UNDO. +PROCEDURE inner: + DEFINE INPUT-OUTPUT PARAMETER dTotal AS DECIMAL NO-UNDO. +END PROCEDURE. +"; + +#[test] +fn a_program_and_its_procedures_have_their_own_lists() { + let files = [("/src/two.p", TWO)]; + let main = signature(&files, "two.p", None).expect("the main block shows its list"); + assert_eq!(main.params.len(), 2); + assert_eq!(main.params[0].mode, ParamMode::Input); + assert_eq!( + main.params[1].shape, + ParamShape::Scalar(Some(PrimitiveTy::Character)) + ); + assert!(!main.from_include); + let inner = signature(&files, "two", Some("INNER")).expect("the procedure has a list"); + assert_eq!(inner.params.len(), 1); + assert_eq!(inner.params[0].mode, ParamMode::InputOutput); +} + +#[test] +fn a_name_the_index_cannot_answer_has_no_list() { + let files = [("/src/two.p", TWO)]; + assert!(signature(&files, "missing.p", None).is_none()); + assert!(signature(&files, "two.p", Some("nope")).is_none()); + let broken = [("/src/broken.p", "IF THEN THEN ELSE.\n")]; + assert!(signature(&broken, "broken.p", None).is_none()); + let class = [("/src/c.cls", "CLASS c:\nEND CLASS.\n")]; + assert!(signature(&class, "c.cls", None).is_none()); +} + +#[test] +fn an_include_that_adds_parameters_marks_the_list() { + let files = [ + ( + "/src/prog.p", + "{head.i}\nDEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n", + ), + ( + "/src/head.i", + "DEFINE INPUT PARAMETER x AS CHARACTER NO-UNDO.\nDEFINE INPUT PARAMETER y AS CHARACTER NO-UNDO.\n", + ), + ]; + let list = signature(&files, "prog.p", None).unwrap(); + assert_eq!(list.params.len(), 3); + assert!(list.from_include); +} + +#[test] +fn an_include_that_adds_none_leaves_the_list_unmarked() { + let files = [ + ( + "/src/prog.p", + "{head.i}\nDEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n", + ), + ("/src/head.i", "DEFINE VARIABLE v AS INTEGER NO-UNDO.\n"), + ]; + let list = signature(&files, "prog.p", None).unwrap(); + assert_eq!(list.params.len(), 1); + assert!(!list.from_include); +} + +#[test] +fn an_include_that_does_not_arrive_leaves_the_list_unknown() { + let files = [( + "/src/prog.p", + "{gone.i}\nDEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n", + )]; + assert!(signature(&files, "prog.p", None).is_none()); +} + +#[test] +fn the_lists_are_read_when_asked_and_not_before() { + // An index that is asked nothing about parameters reads no file for them: + // a name with no file answers `None` without a read of any other. + let files = [("/src/two.p", TWO)]; + assert!(signature(&files, "other.p", None).is_none()); +} + +#[test] +fn the_analysed_file_is_not_its_own_callee() { + let mut fs = InMemoryFileSystem::new(); + fs.insert(PathBuf::from("/src/two.p"), TWO); + let paths = [PathBuf::from("/src")]; + let index = BatchIndex::new(&fs, &paths); + let own = PathBuf::from("/src/two.p"); + let view = index.excluding(Some(&own)); + assert!(view.signature(&IndexName::new("two.p"), None).is_none()); + // Another file's view sees it. + let other = PathBuf::from("/src/caller.p"); + let view = index.excluding(Some(&other)); + assert!(view.signature(&IndexName::new("two.p"), None).is_some()); +} + +#[test] +fn a_seeded_index_lists_the_procedures_the_workspace_defines() { + let mut fs = InMemoryFileSystem::new(); + fs.insert( + PathBuf::from("/src/super.p"), + "PROCEDURE shared-thing:\nEND PROCEDURE.\nproc abbreviated:\nEND PROCEDURE.\n", + ); + fs.insert(PathBuf::from("/src/other.p"), "MESSAGE 1.\n"); + let paths = [PathBuf::from("/src")]; + let files = [PathBuf::from("/src/super.p"), PathBuf::from("/src/other.p")]; + let seeded = BatchIndex::new(&fs, &paths).seeded_with(&files); + assert_eq!( + seeded.workspace_defines_procedure(&IndexName::new("Shared-Thing")), + Some(true) + ); + assert_eq!( + seeded.workspace_defines_procedure(&IndexName::new("abbreviated")), + Some(true) + ); + assert_eq!( + seeded.workspace_defines_procedure(&IndexName::new("absent")), + Some(false) + ); + // An index that holds no file list cannot say. + let plain = BatchIndex::new(&fs, &paths); + assert_eq!( + plain.workspace_defines_procedure(&IndexName::new("x")), + None + ); +} diff --git a/crates/oxabl_lint/src/lib.rs b/crates/oxabl_lint/src/lib.rs index 55226c4d..9996eab8 100644 --- a/crates/oxabl_lint/src/lib.rs +++ b/crates/oxabl_lint/src/lib.rs @@ -43,6 +43,8 @@ //! |-------|-----------------------------|-----------------------|-----------------------------------------------------------| //! | CL001 | [`run_value`] | `strict.calls` | `RUN VALUE(...)` | //! | CL002 | [`dynamic_invoke`] | `strict.calls` | `DYNAMIC-FUNCTION`, `DYNAMIC-INVOKE`, `DYNAMIC-NEW`, `DYNAMIC-PROPERTY`, `CREATE CALL` | +//! | CL003 | [`signature_mismatch`] | `strict.calls` | a static `RUN` whose arguments differ from the callee's parameters in count, kind, mode or type | +//! | CL004 | [`unresolved_call`] | `strict.calls` | a static `RUN` of a name that is neither a procedure of the file nor a program on the search paths | //! | CL005 | [`shared_state`] | `strict.calls` | `NEW SHARED`, `NEW GLOBAL SHARED` and `SHARED` definitions | //! | TX006 | [`missing_no_undo`] | `strict.transactions` | variable, parameter, temp-table or property without `NO-UNDO` | //! | HD001 | [`handle_not_deleted`] | `strict.handles` | a dynamic object that some path loses without a delete | @@ -95,15 +97,16 @@ pub use registry::{ Rule, RuleFn, RuleGroup, RuleMeta, ScopesFn, Stability, groups, }; pub use rules::{ - CL001, CL002, CL005, DB001, HD001, HD002, HD003, HD004, HD005, LINT0001, LINT0002, LINT0003, - LINT0004, LINT0005, LINT0006, LK001, LK002, LK003, NL001, NL002, NL003, NL004, TX001, TX002, - TX003, TX004, TX005, TX006, assigned_but_never_read, block_var_used_outside, default_lock, - delete_not_in_finally, dynamic_invoke, field_after_failed_find, find_in_loop, - handle_not_deleted, implicit_transaction, lock_outlives_transaction, lock_upgrade_in_loop, - memptr_not_freed, missing_no_undo, missing_widget_pool, nullable_db_field, - persistent_procedure_leak, procedure_wide_transaction, run_value, scope_widened, shared_state, - transaction_spans_call, transaction_spans_ui, type_mismatch_assignment, undefined_symbol, - unknown_in_comparison, unknown_propagation, unknown_table_or_field, unused_variable, + CL001, CL002, CL003, CL004, CL005, DB001, HD001, HD002, HD003, HD004, HD005, LINT0001, + LINT0002, LINT0003, LINT0004, LINT0005, LINT0006, LK001, LK002, LK003, NL001, NL002, NL003, + NL004, TX001, TX002, TX003, TX004, TX005, TX006, assigned_but_never_read, + block_var_used_outside, default_lock, delete_not_in_finally, dynamic_invoke, + field_after_failed_find, find_in_loop, handle_not_deleted, implicit_transaction, + lock_outlives_transaction, lock_upgrade_in_loop, memptr_not_freed, missing_no_undo, + missing_widget_pool, nullable_db_field, persistent_procedure_leak, procedure_wide_transaction, + run_value, scope_widened, shared_state, signature_mismatch, transaction_spans_call, + transaction_spans_ui, type_mismatch_assignment, undefined_symbol, unknown_in_comparison, + unknown_propagation, unknown_table_or_field, unresolved_call, unused_variable, }; pub use shared::{CalleeSummaries, RunShared}; diff --git a/crates/oxabl_lint/src/registry.rs b/crates/oxabl_lint/src/registry.rs index 403fb2ea..58418ec8 100644 --- a/crates/oxabl_lint/src/registry.rs +++ b/crates/oxabl_lint/src/registry.rs @@ -13,15 +13,16 @@ use oxabl_scope::FileScopes; use oxabl_semantic::{AnalysisContext, Semantic}; use crate::rules::{ - CL001, CL002, CL005, DB001, HD001, HD002, HD003, HD004, HD005, LINT0001, LINT0002, LINT0003, - LINT0004, LINT0005, LINT0006, LK001, LK002, LK003, NL001, NL002, NL003, NL004, TX001, TX002, - TX003, TX004, TX005, TX006, assigned_but_never_read, block_var_used_outside, default_lock, - delete_not_in_finally, dynamic_invoke, field_after_failed_find, find_in_loop, - handle_not_deleted, implicit_transaction, lock_outlives_transaction, lock_upgrade_in_loop, - memptr_not_freed, missing_no_undo, missing_widget_pool, nullable_db_field, - persistent_procedure_leak, procedure_wide_transaction, run_value, scope_widened, shared_state, - transaction_spans_call, transaction_spans_ui, type_mismatch_assignment, undefined_symbol, - unknown_in_comparison, unknown_propagation, unknown_table_or_field, unused_variable, + CL001, CL002, CL003, CL004, CL005, DB001, HD001, HD002, HD003, HD004, HD005, LINT0001, + LINT0002, LINT0003, LINT0004, LINT0005, LINT0006, LK001, LK002, LK003, NL001, NL002, NL003, + NL004, TX001, TX002, TX003, TX004, TX005, TX006, assigned_but_never_read, + block_var_used_outside, default_lock, delete_not_in_finally, dynamic_invoke, + field_after_failed_find, find_in_loop, handle_not_deleted, implicit_transaction, + lock_outlives_transaction, lock_upgrade_in_loop, memptr_not_freed, missing_no_undo, + missing_widget_pool, nullable_db_field, persistent_procedure_leak, procedure_wide_transaction, + run_value, scope_widened, shared_state, signature_mismatch, transaction_spans_call, + transaction_spans_ui, type_mismatch_assignment, undefined_symbol, unknown_in_comparison, + unknown_propagation, unknown_table_or_field, unresolved_call, unused_variable, }; use crate::shared::RunShared; @@ -213,6 +214,7 @@ enum Body { NullValues(NullsFn), NullFields(NullsFn), Shared(fn(&[Statement], &AnalysisContext, &RunShared) -> Vec), + Calls(fn(&[Statement], &Semantic, &AnalysisContext, &RunShared) -> Vec), } /// A [`Rule`] made of metadata and a plain function — how every built-in rule @@ -326,6 +328,7 @@ impl Rule for FnRule { ) -> Vec { match self.body { Body::Shared(check) => check(program, ctx, shared), + Body::Calls(check) => check(program, sem, ctx, shared), Body::Plain(run) => run(program, sem, ctx), Body::Handles(check) => check(shared.handles(program, ctx), ctx), Body::Scopes(check) => check(shared.scopes(program, ctx), program, ctx), @@ -489,6 +492,36 @@ pub const BUILTIN_RULES: &[FnRule] = &[ dynamic_invoke::DEFAULT_SEVERITY, dynamic_invoke::run, ), + FnRule { + meta: RuleMeta::new( + CL003, + signature_mismatch::NAME, + signature_mismatch::GROUP, + signature_mismatch::DESCRIPTION, + ) + .with_default_severity(signature_mismatch::DEFAULT_SEVERITY) + .with_stability(Stability::Preview) + .context_free() + .reported_at_use_site(), + body: Body::Calls(|program, sem, ctx, shared| { + signature_mismatch::check(program, sem, ctx, shared) + }), + }, + FnRule { + meta: RuleMeta::new( + CL004, + unresolved_call::NAME, + unresolved_call::GROUP, + unresolved_call::DESCRIPTION, + ) + .with_default_severity(unresolved_call::DEFAULT_SEVERITY) + .with_stability(Stability::Preview) + .context_free() + .reported_at_use_site(), + body: Body::Calls(|program, _sem, ctx, shared| { + unresolved_call::check(program, ctx, shared) + }), + }, strict_rule( CL005, shared_state::NAME, diff --git a/crates/oxabl_lint/src/rules/call_support.rs b/crates/oxabl_lint/src/rules/call_support.rs new file mode 100644 index 00000000..ffdb559c --- /dev/null +++ b/crates/oxabl_lint/src/rules/call_support.rs @@ -0,0 +1,50 @@ +//! What the call rules (CL003 and CL004) share: the static `RUN` statements of +//! a file with the call each one makes. + +use oxabl_ast::{Span, Statement, StatementKind}; +use oxabl_index::calls::{CallSite, FileCalls}; +use oxabl_semantic::AnalysisContext; + +use super::syntactic::{Visitor, walk_block}; + +/// Call `f` for every `RUN` statement of `program` that the call graph lists, +/// with its call. +pub(super) fn for_each_run( + program: &[Statement], + calls: &FileCalls, + f: impl FnMut(&Statement, &CallSite), +) { + struct Runs<'a, F> { + calls: &'a FileCalls, + f: F, + } + impl Visitor for Runs<'_, F> { + fn statement(&mut self, stmt: &Statement) -> bool { + if matches!(stmt.kind, StatementKind::Run { .. }) + && let Ok(i) = self + .calls + .sites + .binary_search_by_key(&(stmt.span.start, stmt.span.end), |s| { + (s.at.start, s.at.end) + }) + { + (self.f)(stmt, &self.calls.sites[i]); + } + true + } + } + walk_block(&mut Runs { calls, f }, program); +} + +/// Whether the preprocessor left a gap inside `span`: the compiler may see +/// text there. +pub(super) fn holds_elided(ctx: &AnalysisContext, span: Span) -> bool { + let from = ctx.elided.partition_point(|&at| at < span.start); + ctx.elided.get(from).is_some_and(|&at| at <= span.end) +} + +/// A `RUN` target with no path or extension: it may be an internal procedure +/// that the file's includer or a super procedure supplies. +pub(super) fn is_bare(name: &str) -> bool { + !name.contains(['.', '/', '\\']) +} diff --git a/crates/oxabl_lint/src/rules/mod.rs b/crates/oxabl_lint/src/rules/mod.rs index 3d22c875..ec7c7a72 100644 --- a/crates/oxabl_lint/src/rules/mod.rs +++ b/crates/oxabl_lint/src/rules/mod.rs @@ -23,6 +23,7 @@ pub mod procedure_wide_transaction; pub mod run_value; pub mod scope_widened; pub mod shared_state; +pub mod signature_mismatch; pub mod transaction_spans_call; pub mod transaction_spans_ui; pub mod type_mismatch_assignment; @@ -30,9 +31,12 @@ pub mod undefined_symbol; pub mod unknown_in_comparison; pub mod unknown_propagation; pub mod unknown_table_or_field; +pub mod unresolved_call; pub mod unused_symbol_shared; pub mod unused_variable; +// The `RUN` statements and call-graph entries the call rules read. +mod call_support; // Wording and anchoring for the handle rules. mod handle_support; // Shared queries over the scope analysis for the lock and transaction rules. @@ -57,6 +61,10 @@ pub const LINT0006: &str = "LINT0006"; pub const CL001: &str = "CL001"; /// Diagnostic code for `dynamic-invoke` (CL002). pub const CL002: &str = "CL002"; +/// Diagnostic code for `signature-mismatch` (CL003). +pub const CL003: &str = "CL003"; +/// Diagnostic code for `unresolved-call` (CL004). +pub const CL004: &str = "CL004"; /// Diagnostic code for `shared-state` (CL005). pub const CL005: &str = "CL005"; /// Diagnostic code for `implicit-transaction` (TX001). diff --git a/crates/oxabl_lint/src/rules/signature_mismatch.rs b/crates/oxabl_lint/src/rules/signature_mismatch.rs new file mode 100644 index 00000000..7692d7f6 --- /dev/null +++ b/crates/oxabl_lint/src/rules/signature_mismatch.rs @@ -0,0 +1,491 @@ +//! `signature-mismatch` lint (CL003), group `strict.calls`. +//! +//! Fires on a static `RUN` whose arguments do not fit the parameters of the +//! procedure it runs: a different count, another kind or mode, or a handle +//! where a value of another type is expected. +//! +//! # Why +//! +//! ```abl +//! PROCEDURE total-order: +//! DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. +//! DEFINE OUTPUT PARAMETER dTotal AS DECIMAL NO-UNDO. +//! END PROCEDURE. +//! DEFINE VARIABLE dTotal AS DECIMAL NO-UNDO. +//! RUN total-order (INPUT 1, dTotal). +//! ``` +//! +//! `dTotal` is passed as an `INPUT` argument where the procedure writes an +//! `OUTPUT` parameter. A procedure in the same file is checked when the file +//! compiles. A program run from another file is checked only when the call +//! runs, so a signature that changed after the caller was written fails in +//! production. +//! +//! Fixed: +//! +//! ```abl +//! PROCEDURE total-order: +//! DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. +//! DEFINE OUTPUT PARAMETER dTotal AS DECIMAL NO-UNDO. +//! END PROCEDURE. +//! DEFINE VARIABLE dTotal AS DECIMAL NO-UNDO. +//! RUN total-order (INPUT 1, OUTPUT dTotal). +//! ``` +//! +//! # What is and is not reported +//! +//! The callee must be known whole. A `RUN` is checked when it runs, without +//! `ASYNCHRONOUS` or `ON SERVER`: +//! +//! - an internal procedure of the file, also `IN THIS-PROCEDURE`; +//! - a program found on the search paths (`RUN post-order.p`, also +//! `PERSISTENT`): its parameters are the `DEFINE ... PARAMETER` statements of +//! its main block; +//! - a procedure of a program the file runs `PERSISTENT SET h`, called `IN h` +//! when every `PERSISTENT SET h` of the file runs one program. +//! +//! Per `RUN`, a count that differs gives one diagnostic on the target. With +//! the same count, each argument gives at most one diagnostic, the first that +//! applies: +//! +//! 1. the kind: a `TABLE`, `TABLE-HANDLE`, `DATASET`, `DATASET-HANDLE`, +//! `BUFFER` or plain argument where the parameter is another kind. A +//! `TABLE` and a `TABLE-HANDLE` fit each other, as do a `DATASET` and a +//! `DATASET-HANDLE`; +//! 2. the mode: the argument's `INPUT`, `OUTPUT` or `INPUT-OUTPUT` (`INPUT` +//! when none is written) differs from the parameter's mode. A `BUFFER` +//! parameter has no mode; +//! 3. an expression (a literal, an operator, `IF ... THEN ... ELSE`, `NEW`) +//! passed as `OUTPUT` or `INPUT-OUTPUT`; +//! 4. the data type, only where a handle meets a type it has no conversion +//! to or from: a `HANDLE` or `COM-HANDLE` on one side and, on the other, a +//! type other than `HANDLE`, `COM-HANDLE`, `INTEGER`, `INT64`, `CHARACTER` +//! or `LONGCHAR`. An `INPUT` value goes from the argument to the +//! parameter, an `OUTPUT` value back, and an `INPUT-OUTPUT` value both +//! ways. Every other pair of types may convert when the call runs, so it +//! gives no finding: production code passes an `INTEGER` field to a +//! `CHARACTER` parameter, and a `CHARACTER` value to a `DECIMAL` one. +//! +//! A callee whose parameter list an `{include}` or `&IF` supplies is read from +//! the expanded text, and another compile environment (another search path or +//! `&GLOBAL-DEFINE`, such as a character and a graphical build of one program) +//! may expand it to another list. A call with no text from an include inside +//! it (`RUN p ({globals-out.i}, x)` has some) is then not judged on the +//! count. Kind, mode and type are still compared when the counts agree. +//! +//! Never reported, because the callee or an argument is Unknown: +//! +//! - a target the call graph does not resolve (`RUN VALUE(...)`, a handle +//! the file does not set once, a program not on the search paths, a name +//! several files match, a file that does not parse); +//! - a callee whose parameter list an `{include}` that did not arrive, or an +//! `&IF` that holds a parameter, may change; +//! - a parameter or argument of unknown type: `LIKE`, a class, an array +//! (`EXTENT`), an unresolved name, the unknown value `?`; +//! - a `RUN` whose argument list holds an include reference or a gap the +//! preprocessor left; +//! - in an include checked alone, and in a file with an include that did not +//! arrive, a bare name that may be a procedure of the includer. +//! +//! The `TABLE` parameter's temp-table definition and the `BIND`, `APPEND` and +//! `BY-REFERENCE` options are not compared. + +use oxabl_ast::{ + Expression, ExpressionKind, ParameterDirection, RunArgument, RunTarget, Span, Statement, + StatementKind, +}; +use oxabl_common::{Diagnostic, Severity}; +use oxabl_index::calls::{CallKind, CallTarget}; +use oxabl_lexer::{Kind, Token}; +use oxabl_semantic::{ + AnalysisContext, IndexName, Param, ParamMode, ParamShape, PrimitiveTy, ProgramSignatures, + ResolvedType, Semantic, Signature, +}; +use std::sync::Arc; + +use super::CL003; +use super::call_support::{for_each_run, holds_elided, is_bare}; +use super::syntactic::file_span; +use crate::shared::RunShared; + +/// Kebab-case rule name. +pub const NAME: &str = "signature-mismatch"; +/// Rule group, enabled together with its siblings. +pub const GROUP: &str = "strict.calls"; +/// Severity when the config does not set one. +pub const DEFAULT_SEVERITY: Severity = Severity::Warning; +/// One-line description for listings. +pub const DESCRIPTION: &str = + "A RUN whose arguments differ from the callee's parameters in count, kind, mode or type"; + +/// Entry point without shared facts. +pub fn run(program: &[Statement], sem: &Semantic, ctx: &AnalysisContext) -> Vec { + check(program, sem, ctx, &RunShared::default()) +} + +/// Shared entry point. +pub fn check( + program: &[Statement], + sem: &Semantic, + ctx: &AnalysisContext, + shared: &RunShared, +) -> Vec { + if ctx.parse_recovered { + return Vec::new(); + } + let tokens = shared.tokens(ctx); + let fragment = ctx.source_context.is_include_fragment(); + let mut local: Option = None; + let mut diags = Vec::new(); + for_each_run(program, shared.calls(program, ctx), |stmt, site| { + if !matches!(site.kind, CallKind::Run | CallKind::Persistent) { + return; + } + let StatementKind::Run { + target: RunTarget::Literal { name_span, .. }, + arguments, + .. + } = &stmt.kind + else { + return; + }; + let signature: Option> = match &site.target { + CallTarget::Procedure(name) => { + if (fragment && is_bare(name)) || !ctx.elided_includes.is_empty() { + return; + } + let sigs = local.get_or_insert_with(|| ProgramSignatures::of(program, tokens)); + sigs.procedure(&IndexName::new(name)).cloned().map(Arc::new) + } + CallTarget::Program(name) => { + if !ctx.index_loaded + || (is_bare(name) && (fragment || !ctx.elided_includes.is_empty())) + { + return; + } + ctx.index.signature(&IndexName::new(name), None) + } + CallTarget::InProgram { program, procedure } => { + if !ctx.index_loaded { + return; + } + ctx.index + .signature(&IndexName::new(program), Some(&IndexName::new(procedure))) + } + _ => None, + }; + let Some(signature) = signature else { + return; + }; + if holds_elided(ctx, stmt.span) || arguments.iter().any(hides_arguments) { + return; + } + let name = match &site.target { + CallTarget::Procedure(n) | CallTarget::Program(n) => n.as_str(), + CallTarget::InProgram { procedure, .. } => procedure.as_str(), + _ => "", + }; + compare( + &Callee { + name, + target: *name_span, + signature: &signature, + include_in_call: include_in_call(ctx, stmt.span), + }, + arguments, + sem, + ctx, + tokens, + &mut diags, + ); + }); + diags +} + +/// Whether an argument is text the parser cannot count: an include or a +/// preprocessor reference. +fn hides_arguments(arg: &RunArgument) -> bool { + matches!( + arg.expression.kind, + ExpressionKind::IncludeReference { .. } + | ExpressionKind::IncludeArgReference { .. } + | ExpressionKind::PreprocReference(_) + | ExpressionKind::PreprocIf(_) + ) +} + +struct Callee<'a> { + name: &'a str, + /// Where the call names its target. + target: Span, + signature: &'a Signature, + /// An include file wrote part of the call's text. + include_in_call: bool, +} + +fn compare( + callee: &Callee, + arguments: &[RunArgument], + sem: &Semantic, + ctx: &AnalysisContext, + tokens: &[Token], + diags: &mut Vec, +) { + let params = &callee.signature.params; + let name = callee.name; + if arguments.len() != params.len() { + // An include supplies some of the callee's parameters and the call names + // none from an include: another environment compiles the callee to + // another list, so the count proves nothing. + if callee.signature.from_include && !callee.include_in_call { + return; + } + diags.push(report( + ctx, + callee.target, + format!( + "`RUN {name}` passes {} {}, but the procedure takes {}", + arguments.len(), + plural(arguments.len(), "argument"), + params.len() + ), + "Pass one argument for each `DEFINE ... PARAMETER`, in order.", + )); + return; + } + for (i, (arg, param)) in arguments.iter().zip(params).enumerate() { + let n = i + 1; + let span = arg.expression.span; + let shape = argument_shape(tokens, &arg.expression); + if !shapes_fit(shape, param.shape) { + diags.push(report( + ctx, + span, + format!( + "argument {n} of `RUN {name}` is {}, but the parameter is {}", + shape_name(shape), + shape_name(param.shape) + ), + "Pass the argument in the form the parameter is defined in.", + )); + continue; + } + let mode = argument_mode(&arg.direction); + if param.shape != ParamShape::Buffer && mode != param.mode { + diags.push(report( + ctx, + span, + format!( + "argument {n} of `RUN {name}` is passed as {}, but the parameter is {}", + mode_name(mode), + mode_name(param.mode) + ), + "Write the argument's mode to match the parameter: `INPUT`, `OUTPUT` or `INPUT-OUTPUT`.", + )); + continue; + } + if mode != ParamMode::Input && is_expression(&arg.expression) { + diags.push(report( + ctx, + span, + format!( + "argument {n} of `RUN {name}` is an expression, but {} needs a variable or field the procedure can write", + mode_name(mode) + ), + "Pass a variable, field or array element.", + )); + continue; + } + if let Some(message) = type_mismatch(sem, arg, mode, param, n, name) { + diags.push(report( + ctx, + span, + message, + "Pass a value of the parameter's type, or change the parameter.", + )); + } + } +} + +/// Whether an include file wrote part of the text at `span` and not all of +/// it: `RUN p ({globals-out.i}, x)`. A statement that an include holds +/// whole does not count. +fn include_in_call(ctx: &AnalysisContext, span: Span) -> bool { + let from = ctx.include_regions.partition_point(|r| r.end <= span.start); + ctx.include_regions[from..] + .iter() + .take_while(|r| r.start < span.end) + .any(|r| !(r.start <= span.start && r.end >= span.end)) +} + +fn report(ctx: &AnalysisContext, span: Span, message: String, help: &str) -> Diagnostic { + Diagnostic::warning(CL003, message, file_span(ctx, span)).with_help(help.to_string()) +} + +fn plural(n: usize, word: &str) -> String { + if n == 1 { + word.to_string() + } else { + format!("{word}s") + } +} + +/// What the argument is, read from the keyword that precedes or starts it: the +/// parser keeps no record of `TABLE`, `TABLE-HANDLE`, `DATASET`, +/// `DATASET-HANDLE` or `BUFFER` on an argument. +fn argument_shape(tokens: &[Token], expr: &Expression) -> ParamShape { + let start = expr.span.start as usize; + let at = tokens.partition_point(|t| t.start < start); + // `BUFFER b:HANDLE` is a handle value, not a buffer argument. + let plain = matches!(expr.kind, ExpressionKind::Identifier(_)); + if plain + && tokens + .get(at) + .is_some_and(|t| t.start == start && t.kind == Kind::Buffer) + { + return ParamShape::Buffer; + } + match at.checked_sub(1).map(|p| tokens[p].kind) { + Some(Kind::Table) => ParamShape::Table, + Some(Kind::TableHandle) => ParamShape::TableHandle, + Some(Kind::Dataset) => ParamShape::Dataset, + Some(Kind::DatasetHandle) => ParamShape::DatasetHandle, + _ => ParamShape::Scalar(None), + } +} + +fn shapes_fit(arg: ParamShape, param: ParamShape) -> bool { + use ParamShape::*; + match (arg, param) { + (Scalar(_), Scalar(_)) => true, + (Table | TableHandle, Table | TableHandle) => true, + (Dataset | DatasetHandle, Dataset | DatasetHandle) => true, + (a, p) => a == p, + } +} + +fn shape_name(shape: ParamShape) -> &'static str { + match shape { + ParamShape::Scalar(_) => "a plain variable or value", + ParamShape::Buffer => "a `BUFFER`", + ParamShape::Table => "a `TABLE`", + ParamShape::TableHandle => "a `TABLE-HANDLE`", + ParamShape::Dataset => "a `DATASET`", + ParamShape::DatasetHandle => "a `DATASET-HANDLE`", + } +} + +fn argument_mode(direction: &ParameterDirection) -> ParamMode { + match direction { + ParameterDirection::Output => ParamMode::Output, + ParameterDirection::InputOutput => ParamMode::InputOutput, + ParameterDirection::Input | ParameterDirection::Return => ParamMode::Input, + } +} + +fn mode_name(mode: ParamMode) -> &'static str { + match mode { + ParamMode::Input => "`INPUT`", + ParamMode::Output => "`OUTPUT`", + ParamMode::InputOutput => "`INPUT-OUTPUT`", + } +} + +/// Whether `expr` is a value that cannot be written to. +fn is_expression(expr: &Expression) -> bool { + use ExpressionKind::*; + matches!( + expr.kind, + Literal(_) + | Add(..) + | Minus(..) + | Multiply(..) + | Divide(..) + | Modulo(..) + | Equal(..) + | NotEqual(..) + | LessThan(..) + | LessThanOrEqual(..) + | GreaterThan(..) + | GreaterThanOrEqual(..) + | Begins(..) + | Matches(..) + | Contains(..) + | And(..) + | Or(..) + | Negate(_) + | Not(_) + | IfThenElse(..) + | New { .. } + ) +} + +/// The message for an argument whose primitive type cannot convert to the +/// parameter's, or `None` when it may or either side is not known. +fn type_mismatch( + sem: &Semantic, + arg: &RunArgument, + mode: ParamMode, + param: &Param, + n: usize, + name: &str, +) -> Option { + let ParamShape::Scalar(Some(want)) = param.shape else { + return None; + }; + let Some(ResolvedType::Primitive(have)) = sem.types.get(arg.expression.id) else { + return None; + }; + let have = *have; + // A value goes from the argument to the parameter for `INPUT`, back for + // `OUTPUT`, and both ways for `INPUT-OUTPUT`. + let clash = match mode { + ParamMode::Input | ParamMode::Output => handle_clash(have, want), + ParamMode::InputOutput => handle_clash(have, want) || handle_clash(want, have), + }; + clash.then(|| { + format!( + "argument {n} of `RUN {name}` is {}, but the parameter is {}", + type_name(have), + type_name(want) + ) + }) +} + +/// Whether a handle meets a type it has no conversion to or from. Every other +/// pair of scalar types may convert when the call runs: production code passes +/// an `INTEGER` field to a `CHARACTER` parameter, and a `CHARACTER` value to a +/// `DECIMAL` one. +fn handle_clash(a: PrimitiveTy, b: PrimitiveTy) -> bool { + use PrimitiveTy::*; + let handle = |t| matches!(t, Handle | ComHandle); + let converts_with_handle = |t| { + matches!( + t, + Handle | ComHandle | Integer | Int64 | Character | Longchar + ) + }; + (handle(a) || handle(b)) && !(converts_with_handle(a) && converts_with_handle(b)) +} + +fn type_name(ty: PrimitiveTy) -> &'static str { + match ty { + PrimitiveTy::Integer => "INTEGER", + PrimitiveTy::Int64 => "INT64", + PrimitiveTy::Decimal => "DECIMAL", + PrimitiveTy::Character => "CHARACTER", + PrimitiveTy::Longchar => "LONGCHAR", + PrimitiveTy::Logical => "LOGICAL", + PrimitiveTy::Date => "DATE", + PrimitiveTy::Datetime => "DATETIME", + PrimitiveTy::DatetimeTz => "DATETIME-TZ", + PrimitiveTy::Handle => "HANDLE", + PrimitiveTy::Rowid => "ROWID", + PrimitiveTy::Recid => "RECID", + PrimitiveTy::Raw => "RAW", + PrimitiveTy::Memptr => "MEMPTR", + PrimitiveTy::Clob => "CLOB", + PrimitiveTy::Blob => "BLOB", + PrimitiveTy::ComHandle => "COM-HANDLE", + } +} diff --git a/crates/oxabl_lint/src/rules/unresolved_call.rs b/crates/oxabl_lint/src/rules/unresolved_call.rs new file mode 100644 index 00000000..f6c8e04a --- /dev/null +++ b/crates/oxabl_lint/src/rules/unresolved_call.rs @@ -0,0 +1,213 @@ +//! `unresolved-call` lint (CL004), group `strict.calls`. +//! +//! Fires on a static `RUN name` that nothing in the workspace can run: `name` +//! is not an internal procedure of the file, no program of that name is on the +//! search paths, and no file of the workspace defines a procedure of that name. +//! +//! # Why +//! +//! ```abl +//! DEFINE VARIABLE iOrder AS INTEGER NO-UNDO. +//! RUN post-invoice (INPUT iOrder). +//! ``` +//! +//! `post-invoice` is neither a procedure of this file nor a program the +//! workspace holds. The compiler does not check a `RUN` of another program, so +//! a typo, a renamed program or a missing directory in the search path is a +//! run-time error, found when that line runs. +//! +//! Fixed — the procedure is in the file: +//! +//! ```abl +//! DEFINE VARIABLE iOrder AS INTEGER NO-UNDO. +//! PROCEDURE post-invoice: +//! DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. +//! END PROCEDURE. +//! RUN post-invoice (INPUT iOrder). +//! ``` +//! +//! # What is and is not reported +//! +//! One diagnostic per `RUN`, anchored on the target. The call graph +//! ([`oxabl_index::calls`]) must say the target is `not-found`: the index +//! searched every configured path and nothing supplies the name. These are +//! never reported, because the callee is not known to be absent: +//! +//! - `RUN VALUE(...)`, `RUN p IN h`, `RUN SUPER`, and targets that still hold +//! preprocessor text (the `dynamic`, `handle`, `super` and `unexpanded` +//! reasons); +//! - a name that starts with `..`: it is relative to a directory the search +//! paths do not cover; +//! - a name that several files match, or a file that exists and cannot be +//! read or parsed (`ambiguous`, `unusable`); +//! - `RUN ... ASYNCHRONOUS` and `RUN ... ON SERVER h`: the program may exist on +//! the other side only; +//! - a run with no index, or an index with no search path: nothing was +//! searched; +//! - in an include checked alone, and in a file with an include that did not +//! arrive, a bare name: it may be a procedure of the includer; +//! - in a file that binds a web service port type (`RUN port-type SET h ON +//! SERVER s`), a bare name: it is an operation of the service; +//! - a path into a directory the OpenEdge install ships (`adecomm/`, `adeshar/`, +//! `prodict/`, ...): a workspace that does not configure the install cannot +//! supply it. +//! +//! A bare name (no extension, no path) may also be a procedure of a super +//! procedure or of a persistent procedure, and a plain `RUN name` reaches it +//! at run time. The rule reports a bare name only when the index lists the +//! files of the workspace and none of them defines a `PROCEDURE` of that name, +//! as written in the file or in an include file that a file of the workspace +//! names. A run whose index has no such list reports no bare name. +//! +//! # `LINT0001` already reports some of these +//! +//! `undefined-symbol` (`LINT0001`, always on) reports a `RUN` target that +//! names a file (`RUN post-invoice.p`) when no search path supplies it. This +//! rule does not report that case again. It reports the targets `LINT0001` +//! leaves out: a name with no extension (`RUN post-invoice`), and a name with +//! a path and no extension. A `RUN` is never reported by both rules. + +use oxabl_ast::{RunTarget, Statement, StatementKind}; +use oxabl_common::{Diagnostic, Severity}; +use oxabl_index::calls::{ + CallKind, CallTarget, FileCalls, ProgramAnswer, UnknownCall, resolve_program, +}; +use oxabl_lexer::{Kind, Token}; +use oxabl_semantic::{ + AnalysisContext, IndexName, Semantic, run_miss_is_reported, run_target_climbs_out, +}; + +use super::CL004; +use super::call_support::{for_each_run, holds_elided, is_bare}; +use super::syntactic::file_span; +use crate::shared::RunShared; + +/// Kebab-case rule name. +pub const NAME: &str = "unresolved-call"; +/// Rule group, enabled together with its siblings. +pub const GROUP: &str = "strict.calls"; +/// Severity when the config does not set one. +pub const DEFAULT_SEVERITY: Severity = Severity::Warning; +/// One-line description for listings. +pub const DESCRIPTION: &str = + "A RUN of a name that is neither a procedure of the file nor a program on the search paths"; + +/// Entry point without shared facts. +pub fn run(program: &[Statement], _sem: &Semantic, ctx: &AnalysisContext) -> Vec { + check(program, ctx, &RunShared::default()) +} + +/// Shared entry point. +pub fn check(program: &[Statement], ctx: &AnalysisContext, shared: &RunShared) -> Vec { + if !ctx.index_loaded || !ctx.index_searches_paths || ctx.parse_recovered { + return Vec::new(); + } + let fragment = ctx.source_context.is_include_fragment(); + let calls = shared.calls(program, ctx); + let port_type = binds_port_type(calls, shared.tokens(ctx)); + let mut diags = Vec::new(); + for_each_run(program, calls, |stmt, site| { + let CallTarget::Program(name) = &site.target else { + return; + }; + if !matches!(site.kind, CallKind::Run | CallKind::Persistent) { + return; + } + // `LINT0001` reports a missing file; this rule reports the rest. + if run_miss_is_reported(name) || run_target_climbs_out(name) { + return; + } + if is_bare(name) && (fragment || port_type || !ctx.elided_includes.is_empty()) { + return; + } + if shipped_library(name) { + return; + } + if holds_elided(ctx, stmt.span) { + return; + } + let StatementKind::Run { + target: RunTarget::Literal { name_span, .. }, + .. + } = &stmt.kind + else { + return; + }; + // The parser reads a name up to a digit after a slash and stops + // (`RUN dir/870-recon.p` reads `dir/870`); the text that follows is part of it. + let mut rest = ctx + .source + .get(name_span.end as usize..) + .unwrap_or("") + .chars(); + let continues = match rest.next() { + Some('.') => rest.next().is_some_and(char::is_alphanumeric), + Some(c) => c.is_alphanumeric() || matches!(c, '-' | '_' | '/' | '\\'), + None => false, + }; + if continues { + return; + } + if resolve_program(ctx.index, name) != ProgramAnswer::Unknown(UnknownCall::NotFound) { + return; + } + // A super or persistent procedure may supply a bare name. + if is_bare(name) + && ctx.index.workspace_defines_procedure(&IndexName::new(name)) != Some(false) + { + return; + } + diags.push( + Diagnostic::warning( + CL004, + format!( + "`RUN {name}` names no procedure of this file and no program on the search paths" + ), + file_span(ctx, *name_span), + ) + .with_help( + "Check the name. If it is a program, give it its extension or add its directory \ + to `[workspace.sources].include_paths`; if a super procedure supplies it, run it \ + `IN` the handle." + .to_string(), + ), + ); + }); + diags +} + +/// Directories of the OpenEdge install that programs run by path +/// (`RUN adecomm/_osprint.p`). The install is on the search path of a +/// deployment, and a workspace that does not configure it cannot supply them. +const SHIPPED_DIRECTORIES: [&str; 11] = [ + "adecomm", "adeedit", "adeshar", "adeuib", "adeicon", "adeweb", "adetran", "adm", "adm2", + "prodict", "protools", +]; + +fn shipped_library(name: &str) -> bool { + name.split_once(['/', '\\']).is_some_and(|(dir, _)| { + SHIPPED_DIRECTORIES + .iter() + .any(|d| d.eq_ignore_ascii_case(dir)) + }) +} + +/// Whether the file binds a web service port type: `RUN port-type SET h ON +/// SERVER s`, with no `PERSISTENT`, which starts an AppServer program instead. +/// The operations of that port type are then run by their bare names, and +/// nothing in the workspace defines them. +fn binds_port_type(calls: &FileCalls, tokens: &[Token]) -> bool { + calls + .sites + .iter() + .filter(|site| site.kind == CallKind::Remote) + .any(|site| { + let from = tokens.partition_point(|t| t.start < site.at.start as usize); + let mut words = tokens[from..] + .iter() + .take_while(|t| t.end <= site.at.end as usize) + .map(|t| t.kind); + let kinds: Vec = words.by_ref().collect(); + kinds.contains(&Kind::Set) && !kinds.contains(&Kind::Persistent) + }) +} diff --git a/crates/oxabl_lint/src/shared.rs b/crates/oxabl_lint/src/shared.rs index 58bde620..0331a798 100644 --- a/crates/oxabl_lint/src/shared.rs +++ b/crates/oxabl_lint/src/shared.rs @@ -22,6 +22,7 @@ pub struct RunShared<'a> { share_flow: Cell, handles: OnceCell, tokens: OnceCell>, + calls: OnceCell, flow: OnceCell, scopes: OnceCell, effects: OnceCell, @@ -87,7 +88,7 @@ impl<'a> RunShared<'a> { self.effects.get_or_init(|| { let scopes = self.scopes(program, ctx); let tokens = self.tokens(ctx); - let calls = FileCalls::extract(program, ctx.source, tokens); + let calls = self.calls(program, ctx); let schema = ctx.schema_loaded.then_some(ctx.schema); oxabl_effects::analyze_file( program, @@ -95,13 +96,19 @@ impl<'a> RunShared<'a> { tokens, self.flow(program, ctx), scopes, - &calls, + calls, schema, &EffectOptions { elided: ctx.elided }, ) }) } + /// The calls the file makes and what each one calls, extracted once. + pub fn calls(&self, program: &[Statement], ctx: &AnalysisContext) -> &FileCalls { + self.calls + .get_or_init(|| FileCalls::extract(program, ctx.source, self.tokens(ctx))) + } + /// The failed finds of the file and the field references they reach, /// analysed once over [`scopes`](Self::scopes). pub fn nulls(&self, program: &[Statement], ctx: &AnalysisContext) -> &FileNulls { @@ -153,7 +160,7 @@ impl<'a> RunShared<'a> { }) } - fn tokens<'s>(&'s self, ctx: &'s AnalysisContext) -> &'s [oxabl_lexer::Token] { + pub(crate) fn tokens<'s>(&'s self, ctx: &'s AnalysisContext) -> &'s [oxabl_lexer::Token] { if !ctx.tokens.is_empty() { return ctx.tokens; } diff --git a/crates/oxabl_lint/tests/call_rules/cl003_signature_mismatch.rs b/crates/oxabl_lint/tests/call_rules/cl003_signature_mismatch.rs new file mode 100644 index 00000000..7bf4d6f2 --- /dev/null +++ b/crates/oxabl_lint/tests/call_rules/cl003_signature_mismatch.rs @@ -0,0 +1,449 @@ +//! CL003 `signature-mismatch`. + +use oxabl_lint::signature_mismatch; + +use super::{Setup, lines, messages, run_with}; + +const PROCS: &str = "\ +PROCEDURE total-order: + DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. + DEFINE OUTPUT PARAMETER dTotal AS DECIMAL NO-UNDO. +END PROCEDURE. +PROCEDURE both: + DEFINE INPUT-OUTPUT PARAMETER cText AS CHARACTER NO-UNDO. +END PROCEDURE. +DEFINE VARIABLE iNum AS INTEGER NO-UNDO. +DEFINE VARIABLE dNum AS DECIMAL NO-UNDO. +DEFINE VARIABLE cStr AS CHARACTER NO-UNDO. +DEFINE VARIABLE lFlag AS LOGICAL NO-UNDO. +"; + +fn check(calls: &str) -> Vec { + run_with( + signature_mismatch::run, + &format!("{PROCS}{calls}"), + &Setup::default(), + ) +} + +fn count_lines() -> usize { + PROCS.lines().count() +} + +#[test] +fn a_call_that_fits_is_clean() { + assert!( + check("RUN total-order (INPUT iNum, OUTPUT dNum).\nRUN both (INPUT-OUTPUT cStr).\n") + .is_empty() + ); + // A mode left out means `INPUT`. + assert!(check("RUN total-order (iNum, OUTPUT dNum).\n").is_empty()); +} + +#[test] +fn too_few_or_too_many_arguments_is_one_finding_on_the_target() { + let few = check("RUN total-order (INPUT iNum).\n"); + assert_eq!(few.len(), 1); + assert!( + few[0] + .message + .contains("passes 1 argument, but the procedure takes 2") + ); + let span = few[0].span.span; + let source = format!("{PROCS}RUN total-order (INPUT iNum).\n"); + assert_eq!( + &source[span.start as usize..span.end as usize], + "total-order" + ); + + let many = check("RUN both (INPUT-OUTPUT cStr, INPUT iNum).\n"); + assert_eq!(many.len(), 1); + assert!( + many[0] + .message + .contains("passes 2 arguments, but the procedure takes 1") + ); + + let none = check("RUN both.\n"); + assert!(none[0].message.contains("passes 0 arguments")); +} + +#[test] +fn an_output_parameter_passed_without_output_is_reported() { + let found = check("RUN total-order (INPUT iNum, dNum).\n"); + assert_eq!(found.len(), 1); + assert!(found[0].message.contains("argument 2")); + assert!( + found[0] + .message + .contains("passed as `INPUT`, but the parameter is `OUTPUT`") + ); +} + +#[test] +fn an_input_parameter_passed_as_output_is_reported() { + let found = check("RUN total-order (OUTPUT iNum, OUTPUT dNum).\n"); + assert_eq!(found.len(), 1); + assert!(found[0].message.contains("argument 1")); + assert!( + found[0] + .message + .contains("passed as `OUTPUT`, but the parameter is `INPUT`") + ); +} + +#[test] +fn an_input_output_parameter_needs_input_output() { + assert_eq!(check("RUN both (INPUT cStr).\n").len(), 1); + assert_eq!(check("RUN both (OUTPUT cStr).\n").len(), 1); + assert!(check("RUN both (INPUT-OUTPUT cStr).\n").is_empty()); +} + +#[test] +fn an_expression_passed_as_output_is_reported() { + let found = check("RUN total-order (INPUT 1, OUTPUT dNum + 1).\n"); + assert_eq!(found.len(), 1); + assert!(found[0].message.contains("is an expression")); + let literal = check("RUN total-order (INPUT 1, OUTPUT 5).\n"); + assert_eq!(literal.len(), 1); + // Fields, variables and array elements are writable. + assert!(check("RUN total-order (INPUT 1, OUTPUT dNum).\n").is_empty()); +} + +#[test] +fn only_a_handle_clash_is_a_type_finding() { + let source = "\ +PROCEDURE p: + DEFINE INPUT PARAMETER lOk AS LOGICAL NO-UNDO. + DEFINE INPUT PARAMETER cOk AS CHARACTER NO-UNDO. + DEFINE INPUT PARAMETER iOk AS INTEGER NO-UNDO. + DEFINE OUTPUT PARAMETER hOut AS HANDLE NO-UNDO. +END PROCEDURE. +DEFINE VARIABLE h AS HANDLE NO-UNDO. +DEFINE VARIABLE c AS CHARACTER NO-UNDO. +DEFINE VARIABLE i AS INTEGER NO-UNDO. +DEFINE VARIABLE l AS LOGICAL NO-UNDO. +"; + let run = |calls: &str| { + run_with( + signature_mismatch::run, + &format!("{source}{calls}"), + &Setup::default(), + ) + }; + // A handle for a LOGICAL parameter. + let found = run("RUN p (h, c, i, OUTPUT h).\n"); + assert_eq!(found.len(), 1); + assert!( + found[0] + .message + .contains("is HANDLE, but the parameter is LOGICAL") + ); + // A handle value written back into an INTEGER variable converts; a LOGICAL + // variable does not take it. + assert!(run("RUN p (l, c, i, OUTPUT h).\n").is_empty()); + // Other pairs may convert when the call runs: no finding. + assert!(run("RUN p (i, i, c, OUTPUT h).\n").is_empty()); + assert!(run("RUN p (c, l, h, OUTPUT h).\n").is_empty()); + // The unknown value fits anything. + assert!(run("RUN p (?, ?, ?, OUTPUT h).\n").is_empty()); +} + +#[test] +fn an_argument_gets_at_most_one_finding() { + // A wrong mode, and an expression written as `OUTPUT`: the mode is reported. + let found = check("RUN total-order (OUTPUT 1, OUTPUT dNum).\n"); + assert_eq!(found.len(), 1); + assert!(found[0].message.contains("passed as")); +} + +#[test] +fn findings_are_anchored_on_the_argument() { + let source = format!("{PROCS}RUN total-order (\n INPUT iNum,\n dNum).\n"); + let found = run_with(signature_mismatch::run, &source, &Setup::default()); + assert_eq!(lines(&source, &found), [count_lines() + 3]); +} + +#[test] +fn unknown_types_never_make_a_finding() { + let source = "\ +PROCEDURE p: + DEFINE INPUT PARAMETER a LIKE Customer.Name. + DEFINE INPUT PARAMETER b AS INTEGER EXTENT 3 NO-UNDO. + DEFINE INPUT PARAMETER c AS CLASS Foo.Bar NO-UNDO. +END PROCEDURE. +DEFINE VARIABLE iNum AS INTEGER NO-UNDO. +DEFINE VARIABLE lFlag AS LOGICAL NO-UNDO. +RUN p (INPUT lFlag, INPUT iNum, INPUT iNum). +"; + assert!(run_with(signature_mismatch::run, source, &Setup::default()).is_empty()); +} + +#[test] +fn table_buffer_and_dataset_arguments_are_matched_by_kind() { + let source = "\ +DEFINE TEMP-TABLE tt NO-UNDO FIELD f AS INTEGER. +DEFINE DATASET ds FOR tt. +DEFINE VARIABLE iNum AS INTEGER NO-UNDO. +DEFINE VARIABLE h AS HANDLE NO-UNDO. +PROCEDURE takes-table: + DEFINE INPUT PARAMETER TABLE FOR tt. +END PROCEDURE. +PROCEDURE takes-handle: + DEFINE INPUT PARAMETER TABLE-HANDLE th. +END PROCEDURE. +PROCEDURE takes-dataset: + DEFINE INPUT PARAMETER DATASET FOR ds. +END PROCEDURE. +PROCEDURE takes-buffer: + DEFINE PARAMETER BUFFER b FOR tt. +END PROCEDURE. +"; + let run = |calls: &str| { + run_with( + signature_mismatch::run, + &format!("{source}{calls}"), + &Setup::default(), + ) + }; + // A TABLE and a TABLE-HANDLE fit each other. + assert!(run("RUN takes-table (INPUT TABLE tt).\nRUN takes-handle (INPUT TABLE tt).\nRUN takes-handle (INPUT TABLE-HANDLE h).\nRUN takes-table (INPUT TABLE-HANDLE h).\n").is_empty()); + assert!( + run("RUN takes-dataset (INPUT DATASET ds).\nRUN takes-dataset (INPUT DATASET-HANDLE h).\n") + .is_empty() + ); + assert!(run("RUN takes-buffer (BUFFER tt).\n").is_empty()); + // A plain variable where a table is expected, and the other way round. + let found = run("RUN takes-table (INPUT iNum).\n"); + assert_eq!(found.len(), 1); + assert!( + found[0] + .message + .contains("is a plain variable or value, but the parameter is a `TABLE`") + ); + assert_eq!(run("RUN takes-buffer (INPUT iNum).\n").len(), 1); + assert_eq!(run("RUN takes-handle (INPUT h).\n").len(), 1); + assert_eq!(run("RUN takes-dataset (INPUT TABLE tt).\n").len(), 1); + assert_eq!(run("RUN takes-table (INPUT DATASET ds).\n").len(), 1); + assert_eq!( + messages(&run("RUN takes-table (OUTPUT TABLE tt).\n")), + ["argument 1 of `RUN takes-table` is passed as `OUTPUT`, but the parameter is `INPUT`"] + ); +} + +#[test] +fn only_a_call_that_names_a_whole_known_callee_is_checked() { + let source = "\ +DEFINE VARIABLE c AS CHARACTER NO-UNDO. +DEFINE VARIABLE h AS HANDLE NO-UNDO. +PROCEDURE twice: + DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO. +END PROCEDURE. +PROCEDURE twice: +END PROCEDURE. +PROCEDURE once: + DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO. +END PROCEDURE. +RUN twice. +RUN VALUE(c) (INPUT 1, INPUT 2). +RUN once IN h. +RUN once ASYNCHRONOUS. +RUN once ON SERVER h. +RUN once IN THIS-PROCEDURE. +"; + let found = run_with(signature_mismatch::run, source, &Setup::default()); + // Only the `IN THIS-PROCEDURE` call is checked: no argument for `a`. + assert_eq!(lines(source, &found), [16]); +} + +#[test] +fn a_procedure_with_an_include_is_not_checked() { + let source = "\ +PROCEDURE p: + {params.i} +END PROCEDURE. +RUN p (INPUT 1, INPUT 2). +"; + assert!(run_with(signature_mismatch::run, source, &Setup::default()).is_empty()); +} + +#[test] +fn a_run_with_a_lost_include_is_not_checked() { + let source = format!("{PROCS}RUN both.\n"); + let lost = Setup { + elided_includes: &[0], + ..Setup::default() + }; + assert!(run_with(signature_mismatch::run, &source, &lost).is_empty()); + assert_eq!(check("RUN both.\n").len(), 1); +} + +// --------------------------------------------------------------------------- +// Programs of other files +// --------------------------------------------------------------------------- + +const CALLEE: &str = "\ +DEFINE INPUT PARAMETER iOrder AS INTEGER NO-UNDO. +DEFINE OUTPUT PARAMETER dTotal AS DECIMAL NO-UNDO. +"; + +const CALLER_DECLS: &str = "\ +DEFINE VARIABLE iNum AS INTEGER NO-UNDO. +DEFINE VARIABLE dNum AS DECIMAL NO-UNDO. +DEFINE VARIABLE cStr AS CHARACTER NO-UNDO. +DEFINE VARIABLE h AS HANDLE NO-UNDO. +"; + +fn external(calls: &str, files: &[(&str, &str)]) -> Vec { + let source = format!("{CALLER_DECLS}{calls}"); + let setup = Setup { + files, + ..Setup::default() + }; + run_with(signature_mismatch::run, &source, &setup) +} + +#[test] +fn an_external_program_is_checked_against_its_main_block_parameters() { + let files = [("/src/total.p", CALLEE)]; + assert!(external("RUN total.p (INPUT iNum, OUTPUT dNum).\n", &files).is_empty()); + assert_eq!(external("RUN total.p (INPUT iNum).\n", &files).len(), 1); + assert_eq!( + external("RUN total.p (INPUT iNum, dNum).\n", &files).len(), + 1 + ); + assert_eq!( + external("RUN total.p (INPUT-OUTPUT iNum, OUTPUT dNum).\n", &files).len(), + 1 + ); + // Without the extension, and with PERSISTENT. + assert_eq!(external("RUN total (INPUT iNum).\n", &files).len(), 1); + assert_eq!( + external("RUN total.p PERSISTENT SET h (INPUT iNum).\n", &files).len(), + 1 + ); +} + +#[test] +fn a_program_with_no_parameters_takes_no_arguments() { + let files = [("/src/plain.p", "MESSAGE 1.\n")]; + assert!(external("RUN plain.p.\n", &files).is_empty()); + assert_eq!(external("RUN plain.p (INPUT iNum).\n", &files).len(), 1); +} + +#[test] +fn a_callee_that_is_not_known_gives_no_finding() { + // Not on the search path. + assert!(external("RUN missing.p (INPUT iNum).\n", &[]).is_empty()); + // A parse the file does not survive. + let broken = [( + "/src/broken.p", + "DEFINE INPUT PARAMETER AS AS AS.\nIF THEN THEN.\n", + )]; + assert!(external("RUN broken.p (INPUT iNum).\n", &broken).is_empty()); + // A class. + let class = [("/src/cls.p", "CLASS a.b:\nEND CLASS.\n")]; + assert!(external("RUN cls.p (INPUT iNum).\n", &class).is_empty()); + // Several files. + let setup = Setup { + files: &[("/a/t.p", CALLEE), ("/b/t.p", "MESSAGE 1.\n")], + paths: Some(&["/a", "/b"]), + ..Setup::default() + }; + assert!(run_with(signature_mismatch::run, "RUN t.p.\n", &setup).is_empty()); + // No index. + let none = Setup { + no_index: true, + files: &[("/src/total.p", CALLEE)], + ..Setup::default() + }; + assert!(run_with(signature_mismatch::run, "RUN total.p (INPUT 1).\n", &none).is_empty()); +} + +#[test] +fn an_include_in_the_callee_is_read_through() { + let files = [ + ("/src/total.p", "{params.i}\nMESSAGE 1.\n"), + ("/src/params.i", CALLEE), + ]; + assert!(external("RUN total.p (INPUT iNum, OUTPUT dNum).\n", &files).is_empty()); + // Mode and type are compared as for any callee. + assert_eq!( + external("RUN total.p (INPUT iNum, dNum).\n", &files).len(), + 1 + ); + // An include that is not there leaves the parameters unknown. + let lost = [("/src/total.p", "{missing.i}\nMESSAGE 1.\n")]; + assert!(external("RUN total.p (INPUT iNum).\n", &lost).is_empty()); +} + +#[test] +fn a_count_against_an_include_supplied_list_needs_an_argument_from_an_include() { + let files = [ + ("/src/total.p", "{params.i}\nMESSAGE 1.\n"), + ("/src/params.i", CALLEE), + ]; + // The call names no argument from an include: not judged on the count. + assert!(external("RUN total.p (INPUT iNum).\n", &files).is_empty()); + // With an argument an include wrote, the count is judged. The region is + // the `INPUT iNum` of the source below. + let source = format!("{CALLER_DECLS}RUN total.p (INPUT iNum).\n"); + let at = source.find("INPUT iNum").unwrap() as u32; + let regions = [oxabl_ast::Span { + start: at, + end: at + 10, + }]; + let setup = Setup { + files: &files, + include_regions: ®ions, + ..Setup::default() + }; + assert_eq!(run_with(signature_mismatch::run, &source, &setup).len(), 1); + // A statement that an include holds whole is not a call with an include in it. + let whole = [oxabl_ast::Span { + start: at - 20, + end: at + 30, + }]; + let setup = Setup { + files: &files, + include_regions: &whole, + ..Setup::default() + }; + assert!(run_with(signature_mismatch::run, &source, &setup).is_empty()); + // A list the callee's own text shows is judged whatever the call holds. + let own = [("/src/total.p", CALLEE)]; + assert_eq!(external("RUN total.p (INPUT iNum).\n", &own).len(), 1); +} + +#[test] +fn a_procedure_in_a_persistent_program_is_checked_through_its_handle() { + let lib = "\ +PROCEDURE add-line: + DEFINE INPUT PARAMETER iNum AS INTEGER NO-UNDO. +END PROCEDURE. +"; + let files = [("/src/lib.p", lib)]; + let call = |run: &str| external(&format!("RUN lib.p PERSISTENT SET h.\n{run}"), &files); + assert!(call("RUN add-line IN h (INPUT iNum).\n").is_empty()); + assert_eq!(call("RUN add-line IN h.\n").len(), 1); + assert_eq!(call("RUN add-line IN h (OUTPUT iNum).\n").len(), 1); + // A handle the file sets in another way is Unknown. + let reset = "RUN lib.p PERSISTENT SET h.\nh = SESSION:FIRST-PROCEDURE.\nRUN add-line IN h.\n"; + assert!(external(reset, &files).is_empty()); +} + +#[test] +fn a_bare_name_in_an_include_alone_is_not_checked() { + let files = [("/src/total.p", CALLEE)]; + let setup = Setup { + files: &files, + fragment: true, + ..Setup::default() + }; + assert!(run_with(signature_mismatch::run, "RUN total (INPUT 1).\n", &setup).is_empty()); + // With the extension the callee does not depend on the includer. + assert_eq!( + run_with(signature_mismatch::run, "RUN total.p (INPUT 1).\n", &setup).len(), + 1 + ); +} diff --git a/crates/oxabl_lint/tests/call_rules/cl004_unresolved_call.rs b/crates/oxabl_lint/tests/call_rules/cl004_unresolved_call.rs new file mode 100644 index 00000000..f8b9442f --- /dev/null +++ b/crates/oxabl_lint/tests/call_rules/cl004_unresolved_call.rs @@ -0,0 +1,219 @@ +//! CL004 `unresolved-call`. + +use oxabl_lint::{undefined_symbol, unresolved_call}; + +use super::{Setup, lines, run_with}; + +fn check(source: &str) -> Vec { + run_with(unresolved_call::run, source, &Setup::default()) +} + +#[test] +fn a_bare_name_that_nothing_supplies_is_reported() { + let source = "RUN post-invoice.\n"; + let found = check(source); + assert_eq!(lines(source, &found), [1]); + assert!(found[0].message.contains("RUN post-invoice")); + // Anchored on the target, not the whole statement. + let span = found[0].span.span; + assert_eq!( + &source[span.start as usize..span.end as usize], + "post-invoice" + ); +} + +#[test] +fn a_program_on_the_search_path_is_not_reported() { + let setup = Setup { + files: &[("/src/post-invoice.p", "MESSAGE 1.\n")], + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, "RUN post-invoice.\n", &setup).is_empty()); + // The sibling: the same call with the file missing fires. + assert_eq!(check("RUN post-invoice.\n").len(), 1); +} + +#[test] +fn a_path_without_an_extension_is_reported_when_nothing_supplies_it() { + let source = "RUN orders/post-invoice.\n"; + assert_eq!(check(source).len(), 1); + let setup = Setup { + files: &[("/src/orders/post-invoice.p", "MESSAGE 1.\n")], + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &setup).is_empty()); +} + +#[test] +fn an_internal_procedure_is_not_reported() { + let source = "PROCEDURE post-invoice:\nEND PROCEDURE.\nRUN post-invoice.\nRUN POST-INVOICE.\n"; + assert!(check(source).is_empty()); +} + +#[test] +fn a_file_name_is_left_to_undefined_symbol() { + let source = "RUN missing.p.\nRUN missing.\n"; + let cl004 = check(source); + assert_eq!(lines(source, &cl004), [2]); + // LINT0001 reports the first and not the second, so no `RUN` is reported twice. + let lint0001 = run_with(undefined_symbol::run, source, &Setup::default()); + assert_eq!(lines(source, &lint0001), [1]); +} + +#[test] +fn a_target_the_call_graph_cannot_name_is_not_reported() { + let source = "\ +DEFINE VARIABLE c AS CHARACTER NO-UNDO. +DEFINE VARIABLE h AS HANDLE NO-UNDO. +RUN VALUE(c). +RUN something IN h. +RUN SUPER. +RUN other IN THIS-PROCEDURE. +"; + // Only `IN THIS-PROCEDURE` names an internal procedure that is not there. + let found = check(source); + assert_eq!(lines(source, &found), [6]); + assert!(found[0].message.contains("RUN other")); +} + +#[test] +fn an_asynchronous_or_remote_run_is_not_reported() { + let source = "\ +DEFINE VARIABLE h AS HANDLE NO-UNDO. +RUN remote-thing ASYNCHRONOUS SET h. +RUN other-thing ON SERVER h. +"; + assert!(check(source).is_empty()); +} + +#[test] +fn a_persistent_run_is_reported_like_a_plain_one() { + let source = "DEFINE VARIABLE h AS HANDLE NO-UNDO.\nRUN lib-thing PERSISTENT SET h.\n"; + assert_eq!(check(source).len(), 1); +} + +#[test] +fn a_name_several_files_match_is_not_reported() { + let setup = Setup { + files: &[ + ("/a/thing.p", "MESSAGE 1.\n"), + ("/b/thing.p", "MESSAGE 2.\n"), + ], + paths: Some(&["/a", "/b"]), + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, "RUN thing.\n", &setup).is_empty()); +} + +#[test] +fn nothing_is_reported_when_nothing_was_searched() { + let source = "RUN post-invoice.\n"; + let no_index = Setup { + no_index: true, + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &no_index).is_empty()); + let no_paths = Setup { + paths: Some(&[]), + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &no_paths).is_empty()); +} + +#[test] +fn a_bare_name_in_an_include_alone_or_after_a_lost_include_is_not_reported() { + let source = "RUN post-invoice.\n"; + let fragment = Setup { + fragment: true, + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &fragment).is_empty()); + let lost = Setup { + elided_includes: &[0], + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &lost).is_empty()); + // A path-qualified name does not depend on the includer. + let qualified = Setup { + fragment: true, + ..Setup::default() + }; + assert_eq!( + run_with( + unresolved_call::run, + "RUN orders/post-invoice.\n", + &qualified + ) + .len(), + 1 + ); +} + +#[test] +fn a_bare_name_that_a_file_of_the_workspace_defines_is_not_reported() { + // A super or persistent procedure supplies it at run time. + let source = "RUN setup-globals.\n"; + let setup = Setup { + files: &[( + "/src/super.p", + "PROCEDURE setup-globals:\nEND PROCEDURE.\nproc other-one:\nEND PROCEDURE.\n", + )], + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, source, &setup).is_empty()); + assert!(run_with(unresolved_call::run, "RUN OTHER-ONE.\n", &setup).is_empty()); + // A name no file defines still fires with the same workspace. + assert_eq!( + run_with(unresolved_call::run, "RUN nobody.\n", &setup).len(), + 1 + ); +} + +#[test] +fn a_bare_name_is_not_reported_when_the_index_lists_no_files() { + let setup = Setup { + unseeded: true, + ..Setup::default() + }; + assert!(run_with(unresolved_call::run, "RUN post-invoice.\n", &setup).is_empty()); + // A path-qualified name is a file name and needs no list. + assert_eq!( + run_with(unresolved_call::run, "RUN orders/post-invoice.\n", &setup).len(), + 1 + ); +} + +#[test] +fn a_program_the_openedge_install_ships_is_not_reported() { + assert!(check("RUN adecomm/_osprint.\n").is_empty()); + assert!(check("RUN ADEUIB/_uibmain.\n").is_empty()); + assert_eq!(check("RUN mine/_osprint.\n").len(), 1); +} + +#[test] +fn an_operation_of_a_web_service_is_not_reported() { + let source = "\ +DEFINE VARIABLE hServer AS HANDLE NO-UNDO. +DEFINE VARIABLE hPort AS HANDLE NO-UNDO. +RUN PortSoap SET hPort ON SERVER hServer. +RUN CancelThing (INPUT 1). +"; + assert!(check(source).is_empty()); + // The same call in a file with no port type is reported. + assert_eq!(check("RUN CancelThing (INPUT 1).\n").len(), 1); + // An AppServer program started with PERSISTENT is not a port type. + let appserver = "\ +DEFINE VARIABLE hServer AS HANDLE NO-UNDO. +DEFINE VARIABLE hProc AS HANDLE NO-UNDO. +RUN remote.p PERSISTENT SET hProc ON SERVER hServer. +RUN CancelThing (INPUT 1). +"; + assert_eq!(check(appserver).len(), 1); +} + +#[test] +fn a_name_the_parser_read_short_is_not_reported() { + // The parser stops this name at `dir/870`; the rest is part of the file name. + assert!(check("RUN dir/870-recon-compare.p (INPUT 1).\n").is_empty()); + assert_eq!(check("RUN zf/recon-compare (INPUT 1).\n").len(), 1); +} diff --git a/crates/oxabl_lint/tests/call_rules/doc_examples.rs b/crates/oxabl_lint/tests/call_rules/doc_examples.rs new file mode 100644 index 00000000..b1b1c29a --- /dev/null +++ b/crates/oxabl_lint/tests/call_rules/doc_examples.rs @@ -0,0 +1,74 @@ +//! The `abl` blocks of CL003 and CL004's module docs: the first is the bad +//! example and must fire its rule; the second is the fixed one and must not. + +use oxabl_lint::{signature_mismatch, unresolved_call}; + +use super::{RuleFn, Setup, run_with}; + +/// The `abl` blocks of a module's `//!` docs. +fn abl_blocks(module: &str) -> Vec { + let mut blocks = Vec::new(); + let mut current: Option = None; + for line in module.lines() { + let Some(doc) = line.strip_prefix("//!") else { + continue; + }; + let doc = doc.strip_prefix(' ').unwrap_or(doc); + match (&mut current, doc.trim_end()) { + (None, "```abl") => current = Some(String::new()), + (Some(_), "```") => blocks.extend(current.take()), + (Some(code), _) => { + code.push_str(doc); + code.push('\n'); + } + (None, _) => {} + } + } + blocks +} + +fn examples(name: &str, rule: RuleFn, module: &str) { + let blocks = abl_blocks(module); + assert_eq!( + blocks.len(), + 2, + "{name}: expected a bad and a fixed example" + ); + let setup = Setup::default(); + let bad = run_with(rule, &blocks[0], &setup); + assert!( + !bad.is_empty(), + "{name}: the bad example gives no finding\n{}", + blocks[0] + ); + for (other, f) in [ + ("CL003", signature_mismatch::run as RuleFn), + ("CL004", unresolved_call::run as RuleFn), + ] { + let fixed = run_with(f, &blocks[1], &setup); + assert!( + fixed.is_empty(), + "{name}: the fixed example gives {other} {:?}\n{}", + fixed.iter().map(|d| &d.message).collect::>(), + blocks[1] + ); + } +} + +#[test] +fn cl003_examples() { + examples( + "CL003", + signature_mismatch::run, + include_str!("../../src/rules/signature_mismatch.rs"), + ); +} + +#[test] +fn cl004_examples() { + examples( + "CL004", + unresolved_call::run, + include_str!("../../src/rules/unresolved_call.rs"), + ); +} diff --git a/crates/oxabl_lint/tests/call_rules/main.rs b/crates/oxabl_lint/tests/call_rules/main.rs new file mode 100644 index 00000000..a905be2e --- /dev/null +++ b/crates/oxabl_lint/tests/call_rules/main.rs @@ -0,0 +1,98 @@ +//! CL003 `signature-mismatch` and CL004 `unresolved-call`. +//! +//! The rules read the cross-file index, so the harness drives the real +//! [`BatchIndex`] over an in-memory filesystem rooted at `/src`. Each rule +//! test also asserts a sibling shape that does fire, so a pass cannot come +//! from an inert rule. + +mod cl003_signature_mismatch; +mod cl004_unresolved_call; +mod doc_examples; +mod registry_run; + +use std::path::PathBuf; + +use oxabl_common::{Diagnostic, FileId}; +use oxabl_index::BatchIndex; +use oxabl_lexer::tokenize; +use oxabl_parser::Parser; +use oxabl_schema::Schema; +use oxabl_semantic::{AnalysisContext, SourceContext, analyze_file}; +use oxabl_workspace::InMemoryFileSystem; + +type RuleFn = + fn(&[oxabl_ast::Statement], &oxabl_semantic::Semantic, &AnalysisContext) -> Vec; + +/// How the harness builds the context. +#[derive(Default)] +pub struct Setup<'a> { + /// Files of the workspace, as `(path, text)`. + pub files: &'a [(&'a str, &'a str)], + /// Search paths; `None` is `/src`. + pub paths: Option<&'a [&'a str]>, + /// No index at all. + pub no_index: bool, + /// The index holds no list of the workspace's files. + pub unseeded: bool, + pub fragment: bool, + /// Offsets the preprocessor left a gap at. + pub elided_includes: &'a [u32], + /// The parts of the source that include files wrote. + pub include_regions: &'a [oxabl_ast::Span], +} + +pub fn run_with(rule: RuleFn, source: &str, setup: &Setup) -> Vec { + let mut fs = InMemoryFileSystem::new(); + for (path, text) in setup.files { + fs.insert(PathBuf::from(path), *text); + } + let paths: Vec = match setup.paths { + Some(paths) => paths.iter().map(PathBuf::from).collect(), + None => vec![PathBuf::from("/src")], + }; + let known: Vec = setup.files.iter().map(|(p, _)| PathBuf::from(p)).collect(); + let mut index = BatchIndex::new(&fs, &paths); + if !setup.unseeded { + index = index.seeded_with(&known); + } + let tokens = tokenize(source); + let program = Parser::new(&tokens, source).parse_program(); + assert!( + program.errors.is_empty(), + "parse errors: {:?}\n{source}", + program.errors + ); + let schema = Schema::empty(); + let mut ctx = AnalysisContext::new(FileId::UNKNOWN, source, &schema) + .with_tokens(&tokens) + .with_elided_includes(setup.elided_includes) + .with_include_regions(setup.include_regions) + .with_source_context(if setup.fragment { + SourceContext::IncludeFragment + } else { + SourceContext::CompilationUnit + }); + if !setup.no_index { + ctx = ctx.with_index(&index); + } + let sem = analyze_file(&program.statements, &ctx); + rule(&program.statements, &sem, &ctx) +} + +/// The 1-based line each diagnostic is anchored on. +pub fn lines(source: &str, diags: &[Diagnostic]) -> Vec { + diags + .iter() + .map(|d| { + source[..d.span.span.start as usize] + .bytes() + .filter(|&b| b == b'\n') + .count() + + 1 + }) + .collect() +} + +pub fn messages(diags: &[Diagnostic]) -> Vec<&str> { + diags.iter().map(|d| d.message.as_str()).collect() +} diff --git a/crates/oxabl_lint/tests/call_rules/registry_run.rs b/crates/oxabl_lint/tests/call_rules/registry_run.rs new file mode 100644 index 00000000..f775a9f9 --- /dev/null +++ b/crates/oxabl_lint/tests/call_rules/registry_run.rs @@ -0,0 +1,70 @@ +//! The two rules through the registry: off until the group is enabled. + +use std::path::PathBuf; + +use oxabl_common::{FileId, LintSeverityMap}; +use oxabl_index::BatchIndex; +use oxabl_lexer::tokenize; +use oxabl_lint::{Registry, lint_file}; +use oxabl_parser::Parser; +use oxabl_schema::Schema; +use oxabl_semantic::{AnalysisContext, analyze_file}; +use oxabl_workspace::InMemoryFileSystem; + +const SOURCE: &str = "\ +PROCEDURE p: + DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO. +END PROCEDURE. +RUN p. +RUN missing-one. +"; + +fn codes(severities: LintSeverityMap) -> Vec<&'static str> { + let fs = InMemoryFileSystem::new(); + let paths = [PathBuf::from("/src")]; + let known: [PathBuf; 0] = []; + let index = BatchIndex::new(&fs, &paths).seeded_with(&known); + let tokens = tokenize(SOURCE); + let program = Parser::new(&tokens, SOURCE).parse_program(); + let schema = Schema::empty(); + let ctx = AnalysisContext::new(FileId::UNKNOWN, SOURCE, &schema) + .with_tokens(&tokens) + .with_index(&index) + .with_lint_severities(severities); + let sem = analyze_file(&program.statements, &ctx); + lint_file(&program.statements, &sem, &ctx) + .into_iter() + .map(|d| d.code.0) + .filter(|c| c.starts_with("CL")) + .collect() +} + +#[test] +fn the_default_run_reports_neither_rule() { + assert!(codes(LintSeverityMap::new()).is_empty()); +} + +#[test] +fn enabling_the_group_runs_both() { + let mut severities = LintSeverityMap::new(); + severities.base_mut().enable_group("strict.calls"); + assert_eq!(codes(severities), ["CL003", "CL004"]); +} + +#[test] +fn each_rule_can_be_enabled_alone() { + let mut severities = LintSeverityMap::new(); + severities + .base_mut() + .set_rule("CL004", Some(oxabl_common::Severity::Error)); + assert_eq!(codes(severities), ["CL004"]); +} + +#[test] +fn both_rules_are_registered_in_the_calls_group() { + let registry = Registry::builtin(); + for code in ["CL003", "CL004"] { + let rule = registry.rules().find(|r| r.meta().code == code).unwrap(); + assert_eq!(rule.meta().group, "strict.calls"); + } +} diff --git a/crates/oxabl_lint/tests/registry.rs b/crates/oxabl_lint/tests/registry.rs index 66f4c5d0..67c238d9 100644 --- a/crates/oxabl_lint/tests/registry.rs +++ b/crates/oxabl_lint/tests/registry.rs @@ -96,6 +96,8 @@ fn the_builtin_registry_enumerates_every_rule_with_metadata() { ("LINT0006", "assigned-but-never-read", "default", warning), ("CL001", "run-value", "strict.calls", warning), ("CL002", "dynamic-invoke", "strict.calls", warning), + ("CL003", "signature-mismatch", "strict.calls", warning), + ("CL004", "unresolved-call", "strict.calls", warning), ("CL005", "shared-state", "strict.calls", warning), ("TX006", "missing-no-undo", "strict.transactions", warning), ("HD001", "handle-not-deleted", "strict.handles", warning), @@ -159,7 +161,8 @@ fn the_builtin_registry_enumerates_every_rule_with_metadata() { || m.group == "strict.locks" || m.group == "strict.access" || m.group == "strict.nulls" - || (m.group == "strict.transactions" && m.code != "TX006"), + || (m.group == "strict.transactions" && m.code != "TX006") + || matches!(m.code, "CL003" | "CL004"), "{}", m.code ); diff --git a/crates/oxabl_semantic/src/index.rs b/crates/oxabl_semantic/src/index.rs index 631b0943..5e23871b 100644 --- a/crates/oxabl_semantic/src/index.rs +++ b/crates/oxabl_semantic/src/index.rs @@ -57,7 +57,7 @@ use oxabl_ast::{AccessModifier, Statement, StatementKind, UsingSource}; use oxabl_lexer::oxabl_atom::OxablAtom; use crate::resolve::fold_atom; -use crate::{ResolvedType, SymbolKind}; +use crate::{ResolvedType, Signature, SymbolKind}; // --------------------------------------------------------------------------- // Keys @@ -697,6 +697,31 @@ pub trait WorkspaceIndex { Located::Nowhere } + /// The parameters of the program a literal `RUN` names, or of one of its + /// internal procedures when `procedure` is given. + /// + /// `None` when the index cannot say: no such program, a file it cannot + /// use, a parameter list its statements do not show whole, a procedure + /// the file does not define with a body. A consumer must treat `None` as + /// Unknown, never as "takes no parameters". Defaults to `None`. + fn signature( + &self, + _program: &IndexName, + _procedure: Option<&IndexName>, + ) -> Option> { + None + } + + /// Whether some file of the run's workspace defines an internal procedure + /// called `name`: a procedure that a super procedure or a persistent + /// procedure may supply to a plain `RUN name`. + /// + /// `None` when the index holds no list of the workspace's files, so it + /// cannot say. Defaults to `None`. + fn workspace_defines_procedure(&self, _name: &IndexName) -> Option { + None + } + /// The generation this index is on. Stamped onto every /// [`Semantic`](crate::Semantic) so stale results are detectable. /// Implementations must not return [`IndexRevision::ABSENT`] — that value diff --git a/crates/oxabl_semantic/src/lib.rs b/crates/oxabl_semantic/src/lib.rs index 8f0db2d0..f19bc68e 100644 --- a/crates/oxabl_semantic/src/lib.rs +++ b/crates/oxabl_semantic/src/lib.rs @@ -31,6 +31,7 @@ mod operators; mod resolve; mod scope; mod sequence; +mod signature; mod symbol; mod types; @@ -49,9 +50,13 @@ pub use index::{ pub use index_vec::NodeIndexVec; pub use namespace::{NUM_NAMESPACES, NamespaceId}; pub use operators::{binary_op_result, unary_negate_result, unary_not_result}; -pub use resolve::{Resolution, UnresolvedReason, declare_pass, resolve_pass}; +pub use resolve::{ + Resolution, UnresolvedReason, declare_pass, resolve_pass, run_miss_is_reported, + run_target_climbs_out, +}; pub use scope::{BindingMap, Scope, ScopeId, ScopeKind, ScopeTree}; pub use sequence::{SequenceOperand, sequence_operand}; +pub use signature::{Param, ParamMode, ParamShape, ProgramSignatures, Signature}; pub use symbol::{ ClassLookup, SupertypeRef, Supertypes, Symbol, SymbolFlags, SymbolId, SymbolKind, SymbolTable, UnlinkedRun, diff --git a/crates/oxabl_semantic/src/resolve.rs b/crates/oxabl_semantic/src/resolve.rs index 5cbd9a73..b98e99f2 100644 --- a/crates/oxabl_semantic/src/resolve.rs +++ b/crates/oxabl_semantic/src/resolve.rs @@ -220,6 +220,19 @@ fn climbs_out(raw: &str) -> bool { rest.starts_with("../") || rest.starts_with("..\\") } +/// Whether a literal `RUN` target that no search path supplies is reported as +/// an undefined symbol: it names a file (it has an extension) and does not climb +/// out of the search paths. Any other miss is inconclusive for that rule. +pub fn run_miss_is_reported(raw: &str) -> bool { + has_run_extension(raw) && !climbs_out(raw) +} + +/// Whether a literal `RUN` target starts with `..`, so it is relative to a +/// directory the search paths do not cover and no miss proves anything. +pub fn run_target_climbs_out(raw: &str) -> bool { + climbs_out(raw) +} + /// A `RUN` target that is a single name: no extension and no path separator. fn is_bare_run_name(raw: &str) -> bool { !raw.contains(['/', '\\']) && !has_run_extension(raw) @@ -2002,7 +2015,7 @@ impl<'a> ResolveWalker<'a> { /// new findings were extension-less internal calls, every one of them correct /// ABL. fn run_miss_reason(&self, raw: &str, name: &OxablAtom) -> UnresolvedReason { - if !has_run_extension(raw) || climbs_out(raw) { + if !run_miss_is_reported(raw) { return UnresolvedReason::External; } self.absent_reason(name) diff --git a/crates/oxabl_semantic/src/signature.rs b/crates/oxabl_semantic/src/signature.rs new file mode 100644 index 00000000..c6d33a2f --- /dev/null +++ b/crates/oxabl_semantic/src/signature.rs @@ -0,0 +1,439 @@ +//! Parameter lists, as the caller of a `RUN` sees them. +//! +//! [`ProgramSignatures::of`] reads the `DEFINE ... PARAMETER` statements of an +//! external program's main block and of each internal procedure. A list is +//! only given when the statements show all of it: an `{include}` or an `&IF` +//! that holds a parameter definition makes the list unknown. +//! +//! The semantics, with confidence marks, are in +//! `docs/design/strict-analysis-open-questions.md`, section 10. + +use oxabl_ast::{ + HandleParamKind, ParameterDirection, ParameterType, PreprocIf, Span, Statement, StatementKind, + TypeSource, +}; +use oxabl_lexer::{Kind, Token}; + +use crate::index::IndexName; +use crate::types::{PrimitiveTy, ResolvedType}; + +/// How a parameter passes its value. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ParamMode { + Input, + Output, + InputOutput, +} + +/// What a parameter is. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ParamShape { + /// A variable. The type is `None` when it is not a primitive the + /// declaration states: `LIKE`, a class, a conditional type or an array + /// (`EXTENT`). + Scalar(Option), + /// `BUFFER b FOR table`. + Buffer, + /// `TABLE FOR tt`. + Table, + /// `TABLE-HANDLE h`. + TableHandle, + /// `DATASET FOR ds`. + Dataset, + /// `DATASET-HANDLE h`. + DatasetHandle, +} + +/// One parameter of a procedure. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Param { + pub mode: ParamMode, + pub shape: ParamShape, +} + +/// The parameters of a procedure, in order. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Signature { + pub params: Vec, + /// An `{include}` or an `&IF` supplies some of the parameters: the list + /// comes from the expanded text, and another compile environment (another + /// search path, another `&GLOBAL-DEFINE`) may expand it to another list. + pub from_include: bool, +} + +/// The parameter lists of one file: its main block and its internal +/// procedures. A list is `None` when the file's statements do not show it +/// whole. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct ProgramSignatures { + /// The parameters of the program itself (`RUN file.p (...)`). `None` for a + /// class, interface or a main block with a parameter the statements hide. + pub main: Option, + /// Internal procedures with a body, by folded name. A name defined twice + /// has no list. + pub procedures: Vec<(IndexName, Option)>, + /// How many parameter statements the main block's own text holds. + main_written: usize, + /// The same for each internal procedure. + procedures_written: Vec<(IndexName, usize)>, +} + +impl ProgramSignatures { + /// The parameters `statements` define. `tokens` are the tokens the + /// statements were parsed from. + pub fn of(statements: &[Statement], tokens: &[Token]) -> Self { + let mut out = ProgramSignatures::default(); + let class = statements.iter().any(|s| { + matches!( + s.kind, + StatementKind::Class { .. } | StatementKind::Interface { .. } + ) + }); + // A file with no statement (empty, or all comment) is a placeholder: it + // gives no list. + if class || statements.is_empty() { + return out; + } + let (main, written) = collect(statements, tokens); + out.main = main; + out.main_written = written; + let mut found = Vec::new(); + procedures(statements, tokens, &mut found); + for (name, list, written) in found { + out.procedures.push((name.clone(), list)); + out.procedures_written.push((name, written)); + } + out + } + + /// Mark each list that `written` (the same file's statements before its + /// includes were expanded) did not show whole and that an include added + /// parameters to. + pub fn marking_includes(mut self, written: &ProgramSignatures) -> Self { + if written.main.is_none() + && let Some(main) = &mut self.main + && main.params.len() > written.main_written + { + main.from_include = true; + } + for (name, list) in &mut self.procedures { + let shown = written + .procedures + .iter() + .any(|(n, l)| n == name && l.is_some()); + let own = written + .procedures_written + .iter() + .find(|(n, _)| n == name) + .map_or(0, |(_, n)| *n); + if let (false, Some(list)) = (shown, list) + && list.params.len() > own + { + list.from_include = true; + } + } + self + } + + /// The list of the internal procedure `name`. + pub fn procedure(&self, name: &IndexName) -> Option<&Signature> { + self.procedures + .iter() + .find(|(n, _)| n == name) + .and_then(|(_, s)| s.as_ref()) + } +} + +/// The parameters one block defines, or `None` when something hides part of +/// the list, and how many parameter statements the block's own text holds. +fn collect(statements: &[Statement], tokens: &[Token]) -> (Option, usize) { + let mut params = Vec::new(); + let known = collect_into(statements, tokens, &mut params); + let written = params.len(); + let signature = known.then_some(Signature { + params, + from_include: false, + }); + (signature, written) +} + +/// Add the parameters of `statements` to `params`, looking inside a `DO` block +/// and a label. Returns `false` when something hides part of the list: an +/// include, a conditional or a loop body that holds a parameter. +fn collect_into(statements: &[Statement], tokens: &[Token], params: &mut Vec) -> bool { + let mut known = true; + for s in statements { + match &s.kind { + StatementKind::DefineParameter { + direction, + param_type, + } => { + if let Some(p) = param(direction, param_type, s.span, tokens) { + params.push(p); + } + } + StatementKind::IncludeReference { .. } | StatementKind::IncludeArgReference { .. } => { + known = false; + } + StatementKind::PreprocIf(branches) if hides_parameters(branches) => known = false, + StatementKind::Block(body) => known &= collect_into(body, tokens, params), + StatementKind::Do { + from: None, + to: None, + by: None, + while_condition: None, + on_actions, + body, + .. + } if on_actions.is_empty() => known &= collect_into(body, tokens, params), + StatementKind::Label { body, .. } => { + known &= collect_into(std::slice::from_ref(&**body), tokens, params); + } + // A loop or a condition that defines a parameter: how many + // times it runs is a run-time fact. + _ if holds_parameter(s) => known = false, + _ => {} + } + } + known +} + +/// Whether a statement other than a routine holds a parameter definition in +/// its body. +fn holds_parameter(s: &Statement) -> bool { + let any = |body: &[Statement]| body.iter().any(holds_parameter); + match &s.kind { + StatementKind::DefineParameter { .. } => true, + StatementKind::Do { body, .. } + | StatementKind::Repeat { body, .. } + | StatementKind::ForEach { body, .. } + | StatementKind::Block(body) => any(body), + StatementKind::If { + then_branch, + else_branch, + .. + } => holds_parameter(then_branch) || else_branch.as_deref().is_some_and(holds_parameter), + StatementKind::Case { + when_branches, + otherwise, + .. + } => when_branches.iter().any(|w| any(&w.body)) || otherwise.as_deref().is_some_and(any), + StatementKind::Label { body, .. } => holds_parameter(body), + _ => false, + } +} + +/// Whether a conditional block holds a parameter definition or an include. +fn hides_parameters(branches: &PreprocIf>) -> bool { + let hides = |body: &Vec| { + body.iter().any(|s| match &s.kind { + StatementKind::DefineParameter { .. } + | StatementKind::IncludeReference { .. } + | StatementKind::IncludeArgReference { .. } => true, + StatementKind::PreprocIf(inner) => hides_parameters(inner), + _ => false, + }) + }; + hides(&branches.then_branch) + || branches.elseif_branches.iter().any(|(_, b)| hides(b)) + || branches.else_branch.as_ref().is_some_and(hides) +} + +/// The internal procedures of `statements`, nested `&IF` branches included. +fn procedures( + statements: &[Statement], + tokens: &[Token], + out: &mut Vec<(IndexName, Option, usize)>, +) { + for s in statements { + match &s.kind { + StatementKind::Procedure { + name, + body, + is_prototype: false, + .. + } => { + let key = IndexName::new(&name.name); + if let Some(entry) = out.iter_mut().find(|(n, _, _)| *n == key) { + entry.1 = None; + } else { + let (list, written) = collect(body, tokens); + out.push((key, list, written)); + } + } + StatementKind::PreprocIf(branches) => { + procedures(&branches.then_branch, tokens, out); + for (_, b) in &branches.elseif_branches { + procedures(b, tokens, out); + } + if let Some(b) = &branches.else_branch { + procedures(b, tokens, out); + } + } + _ => {} + } + } +} + +/// One `DEFINE ... PARAMETER` statement as a parameter, or `None` for a +/// `RETURN` parameter, which no argument fills. +fn param( + direction: &ParameterDirection, + param_type: &ParameterType, + span: Span, + tokens: &[Token], +) -> Option { + let mode = match direction { + ParameterDirection::Input => ParamMode::Input, + ParameterDirection::Output => ParamMode::Output, + ParameterDirection::InputOutput => ParamMode::InputOutput, + ParameterDirection::Return => return None, + }; + let shape = match param_type { + ParameterType::Variable { type_source, .. } => { + let array = tokens_in(tokens, span).any(|t| t.kind == Kind::Extent); + ParamShape::Scalar(match type_source { + TypeSource::Explicit(t) if !array => match ResolvedType::from_data_type(t) { + ResolvedType::Primitive(p) => Some(p), + _ => None, + }, + _ => None, + }) + } + ParameterType::Handle { kind, .. } => match kind { + HandleParamKind::Table => ParamShape::Table, + HandleParamKind::TableHandle => ParamShape::TableHandle, + HandleParamKind::Dataset => ParamShape::Dataset, + HandleParamKind::DatasetHandle => ParamShape::DatasetHandle, + }, + ParameterType::Buffer { .. } => ParamShape::Buffer, + }; + Some(Param { mode, shape }) +} + +/// Significant tokens within `span`. +fn tokens_in(tokens: &[Token], span: Span) -> impl Iterator { + let from = tokens.partition_point(|t| t.start < span.start as usize); + tokens[from..] + .iter() + .take_while(move |t| t.end <= span.end as usize && t.kind != Kind::Eof) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn signatures(source: &str) -> ProgramSignatures { + let tokens = oxabl_lexer::tokenize(source); + let program = oxabl_parser::Parser::new(&tokens, source).parse_program(); + ProgramSignatures::of(&program.statements, &tokens) + } + + fn scalar(mode: ParamMode, ty: Option) -> Param { + Param { + mode, + shape: ParamShape::Scalar(ty), + } + } + + #[test] + fn a_program_lists_its_main_block_parameters_in_order() { + let sigs = signatures( + "DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n\ + DEFINE OUTPUT PARAMETER b AS CHARACTER NO-UNDO.\n\ + DEFINE INPUT-OUTPUT PARAMETER TABLE FOR tt.\n\ + DEFINE INPUT PARAMETER pl LIKE Customer.Name.\n\ + DEFINE INPUT PARAMETER arr AS INTEGER EXTENT 3 NO-UNDO.\n", + ); + let main = sigs.main.expect("the main block shows its parameters"); + assert_eq!( + main.params, + [ + scalar(ParamMode::Input, Some(PrimitiveTy::Integer)), + scalar(ParamMode::Output, Some(PrimitiveTy::Character)), + Param { + mode: ParamMode::InputOutput, + shape: ParamShape::Table + }, + scalar(ParamMode::Input, None), + scalar(ParamMode::Input, None), + ] + ); + } + + #[test] + fn an_internal_procedure_has_its_own_list() { + let sigs = signatures( + "PROCEDURE calc:\n DEFINE INPUT PARAMETER x AS DECIMAL NO-UNDO.\nEND PROCEDURE.\n", + ); + assert_eq!(sigs.main, Some(Signature::default())); + let calc = sigs.procedure(&IndexName::new("CALC")).unwrap(); + assert_eq!( + calc.params, + [scalar(ParamMode::Input, Some(PrimitiveTy::Decimal))] + ); + } + + #[test] + fn an_include_hides_the_list() { + let sigs = signatures( + "{params.i}\nPROCEDURE calc:\n {more.i}\nEND PROCEDURE.\n\ + PROCEDURE plain:\n DEFINE INPUT PARAMETER x AS INTEGER NO-UNDO.\nEND PROCEDURE.\n", + ); + assert_eq!(sigs.main, None); + assert_eq!(sigs.procedure(&IndexName::new("calc")), None); + assert!(sigs.procedure(&IndexName::new("plain")).is_some()); + } + + #[test] + fn a_parameter_in_a_do_block_counts_and_one_in_a_loop_or_condition_hides_the_list() { + let sigs = signatures( + "PROCEDURE p:\n DO:\n DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n END.\n\ + END PROCEDURE.\n\ + PROCEDURE q:\n IF TRUE THEN DO:\n DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n END.\n\ + END PROCEDURE.\n\ + PROCEDURE r:\n REPEAT:\n DEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n END.\n\ + END PROCEDURE.\n", + ); + assert_eq!( + sigs.procedure(&IndexName::new("p")).unwrap().params.len(), + 1 + ); + assert_eq!(sigs.procedure(&IndexName::new("q")), None); + assert_eq!(sigs.procedure(&IndexName::new("r")), None); + } + + #[test] + fn a_conditional_parameter_hides_the_list_but_other_conditions_do_not() { + let hidden = signatures( + "&IF DEFINED(x) &THEN\nDEFINE INPUT PARAMETER a AS INTEGER NO-UNDO.\n&ENDIF\n", + ); + assert_eq!(hidden.main, None); + let shown = + signatures("&IF DEFINED(x) &THEN\nDEFINE VARIABLE v AS INTEGER NO-UNDO.\n&ENDIF\n"); + assert_eq!(shown.main, Some(Signature::default())); + } + + #[test] + fn a_procedure_defined_twice_has_no_list() { + let sigs = signatures( + "PROCEDURE p:\n DEFINE INPUT PARAMETER x AS INTEGER NO-UNDO.\nEND PROCEDURE.\n\ + PROCEDURE p:\nEND PROCEDURE.\n", + ); + assert_eq!(sigs.procedure(&IndexName::new("p")), None); + assert_eq!(sigs.procedures.len(), 1); + } + + #[test] + fn a_file_with_no_statement_has_no_list() { + assert_eq!(signatures("").main, None); + assert_eq!(signatures("/* all of it is a comment */").main, None); + assert_eq!(signatures("MESSAGE 1.").main, Some(Signature::default())); + } + + #[test] + fn a_class_has_no_program_signature() { + let sigs = signatures("CLASS a.b:\nEND CLASS.\n"); + assert_eq!(sigs.main, None); + } +} diff --git a/docs/design/strict-analysis-open-questions.md b/docs/design/strict-analysis-open-questions.md index d0b4c529..3e6d4be9 100644 --- a/docs/design/strict-analysis-open-questions.md +++ b/docs/design/strict-analysis-open-questions.md @@ -13,8 +13,9 @@ with the AVM) stays open. Section 4 records one ABL fact that a rule depends on. lists the block semantics that the flow graph (`oxabl_flow`) models without a reference, section 6 the handle semantics that `oxabl_handles` relies on, section 7 the buffer-scope, transaction and lock semantics that `oxabl_scope` relies on, section 8 the database effects, -index selection and call graph that `oxabl_effects` and `oxabl_index` rely on, and section 9 the -unknown-value and record-availability semantics that `oxabl_nulls` relies on. +index selection and call graph that `oxabl_effects` and `oxabl_index` rely on, section 9 the +unknown-value and record-availability semantics that `oxabl_nulls` relies on, and section 10 how +`RUN` matches arguments to parameters and which `RUN` targets the call rules judge. **How to read the confidence marks.** No OpenEdge installation or ABL reference was available while this was written. Each claim is marked: @@ -1489,3 +1490,162 @@ the second (13.4 s and 14.8 s to 14.0 s and 16.0 s); the local solve and NL003 t The field solve runs only over the units that reference a nullable field, and only when a rule that reads it is enabled. One unit of the first corpus runs past its work budget, as it did before this change, and none of the second. + +## 10. Calls and signatures + +The call rules CL003 (`signature-mismatch`) and CL004 (`unresolved-call`) judge a static `RUN` +against its callee. They read the call graph of section 8.5 (`oxabl_index::calls`, which says what +each `RUN` runs, or why that is Unknown) and the parameter lists that the run index now holds +(`WorkspaceIndex::signature`, `oxabl_semantic::ProgramSignatures`). The marks of section 7 apply: +**Documented**, **Recalled**, **Inferred**, **Verified** (against this repository or a corpus), +and **Proposed** for a design choice of oxabl. No OpenEdge install was available, so no row is +checked against a compiler. Where the corpus contradicts what the model first said, the row says +so. + +Each modelled row has a test in `crates/oxabl_lint/tests/call_rules/` (CL003 and CL004 read the +index, so the tests drive a real `BatchIndex` over an in-memory file system), in +`crates/oxabl_index/tests/signatures.rs` and in `oxabl_semantic::signature`. Where the model +cannot decide, the answer is **Unknown**, and Unknown never makes a finding. + +### 10.1 How `RUN` matches arguments to parameters + +| # | The model | Confidence | +|---|---|---| +| P1 | Arguments match parameters by position: the i-th argument fills the i-th `DEFINE ... PARAMETER` of the callee's routine block (the main block of a program, the body of an internal procedure), in source order. Names play no part. A `RETURN` parameter is no argument slot. | Documented. | +| P2 | The counts must be equal. A different count is an error. For an internal procedure of the file the compiler finds it; for another program, a procedure `IN h` or a persistent program it is found when the call runs (the message names "mismatched number of parameters"). | Recalled (the compile-time and run-time split; the message text is from memory). | +| P3 | A mode is `INPUT` (the default when none is written), `OUTPUT` or `INPUT-OUTPUT`. The mode of the argument must be the mode of the parameter: an `OUTPUT` parameter needs `OUTPUT` at the call, an `INPUT` parameter must not get `OUTPUT`, an `INPUT-OUTPUT` parameter needs `INPUT-OUTPUT`. | Recalled. | +| P4 | An `OUTPUT` or `INPUT-OUTPUT` argument must be something the callee can write: a variable, field, array element, widget attribute or property. A literal or an expression (an operator, `IF ... THEN ... ELSE`, `NEW`) is an error. | Documented. | +| P5 | A table, dataset or buffer is passed with its keyword: `TABLE t`, `TABLE-HANDLE h`, `DATASET d`, `DATASET-HANDLE h`, `BUFFER b`. The parameter kinds are `TABLE FOR`, `TABLE-HANDLE`, `DATASET FOR`, `DATASET-HANDLE`, `BUFFER ... FOR`. A static and a dynamic form of one kind pass to each other (`TABLE` to `TABLE-HANDLE`, `DATASET` to `DATASET-HANDLE`). A scalar argument does not pass to a table, dataset or buffer parameter, and the other way round. A `BUFFER` parameter has no mode. `BUFFER b:HANDLE` is a handle value, a scalar. | Recalled. | +| P6 | `APPEND`, `BIND`, `BY-REFERENCE` and `BY-VALUE` change how a table or dataset travels, not whether it fits. The model compares none of them, nor the definition of the temp-table behind a `TABLE`. | Documented for the meaning; Proposed for the scope. | +| P7 | A value of a type that differs from the parameter converts when the call runs, for many pairs. **The corpus shows `INTEGER` fields passed to `CHARACTER` parameters in running code, and `CHARACTER` values passed to `INTEGER`, `LOGICAL`, `DECIMAL` and `DATE` ones.** The first version of CL003 used the assignment ladder of `LINT0004` and gave 470 type findings in one corpus, 340 of them `INTEGER` for `CHARACTER` (section 10.6). So the model keeps one clash: a `HANDLE` or `COM-HANDLE` against a type other than `HANDLE`, `COM-HANDLE`, `INTEGER`, `INT64`, `CHARACTER` or `LONGCHAR`. Every other pair is Unknown. | Verified against the corpus that many pairs convert; the clash itself is Proposed. | +| P8 | The unknown value `?` fits a parameter of any type. | Documented. | +| P9 | A parameter with `LIKE`, an array (`EXTENT`), a class type or a conditional type has no type in the model. An argument without a primitive type (an unresolved name, an array, an object) is Unknown. | Proposed. | +| P10 | A `DEFINE PARAMETER` inside a `DO` block or under a label of the routine is a parameter, in order. One inside a loop or a condition makes the list Unknown. | Verified in the corpus (parameters in a `DO:` block of a procedure are common); Proposed for the rest. | + +### 10.2 Which routine a `RUN` runs + +| # | The model | Confidence | +|---|---|---| +| Q1 | `RUN name` of an internal procedure of the file (with a body, once; a name defined twice has no list) is checked against that procedure. `RUN name IN THIS-PROCEDURE` is the same. | Documented (section 8.5, C1). | +| Q2 | `RUN prog.p` is checked against the main-block parameters of the file that the search path gives (the rule of section 8.5, C2: exactly one match). A class, a file that does not parse and a file with no statement (empty, or all comment) have no list. | Documented for the meaning; Proposed for the file rule. | +| Q3 | `RUN prog.p PERSISTENT SET h (args)` runs the main block now, so the arguments meet the main-block parameters. | Documented. | +| Q4 | `RUN p IN h (args)` is checked against the procedure `p` of the program, when every `PERSISTENT SET h` of the file runs one program and nothing else writes `h` (section 8.5, C3). Any other `h` is Unknown. | Inferred. | +| Q5 | `RUN ... ASYNCHRONOUS` and `RUN ... ON SERVER h` run in another session, and the callee may be another file there: not checked. `RUN VALUE(...)`, `RUN SUPER` and a routine declared `IN SUPER` are Unknown. | Documented for the meaning; Proposed for the scope. | +| Q6 | A user-defined function or a method is a call by name in an expression, not a `RUN`. CL003 does not check them. | Proposed (a later slice). | + +### 10.3 Where a parameter list comes from + +`oxabl_semantic::ProgramSignatures::of` reads the `DEFINE ... PARAMETER` statements of a parsed +file: those of its main block and those of each internal procedure with a body. It also counts the +parameters the text itself holds, so a list that an `{include}` completes is told from one that +the text shows. A list is **Unknown** when an `{include}` (not expanded), an `&IF` that holds a +parameter or a loop that holds one may change it, or when the procedure is defined twice. A file +with no statement has no list. + +The index gives the list through `WorkspaceIndex::signature(program, procedure)`. `BatchIndex` +reads a file for its lists the first time a rule asks, not when it indexes the file, so a run +with CL003 off reads nothing for them. A file whose own text does not show the list whole is read +again from its expanded text (`oxabl_index::program_signatures_at`, the same expansion +`index_file` uses). The daemon holds one tracked query per file and registers the include files, +so an edit to the file or to an include re-reads the list. An index without the method answers +`None`. + +**A list that an include supplies is marked** (`Signature::from_include`), when the expanded text +holds more parameters than the file's own text. The corpus has programs that call each other with +`{globals-out.i}` (72 arguments) against `{globals-in.i}` (72 parameters), and a +character and a graphical build of one program (an include sets a `build-type` name, and +`&IF` then adds the 72 parameters or none). A callee read in one environment can be run in +another. So **a count that differs from an include-supplied list is not judged when the call has +no text from an include**; with an include inside the call (`RUN p ({globals-out.i}, x)`) +the count is judged. Mode, kind and type are compared whenever the counts agree. This row is +**Proposed**; with the rule for an empty program (Q2) it cut the 219 count findings of the first corpus to 157. + +### 10.4 The rule CL004 `unresolved-call` + +A static `RUN name` whose target the call graph says is `not-found`: the index searched every +configured path, and nothing supplies the name, as a program. It is never reported for a +`dynamic`, `handle`, `super`, `unexpanded`, `ambiguous` or `unusable` target, nor with no index or +an index with no search path. + +**Overlap with `LINT0001`.** `LINT0001` (`undefined-symbol`, always on, Error) already reports a +`RUN` target that names a file (it has an extension) when no search path supplies it, except +when the target starts with `..`. CL004 does not report that case again. It reports the rest: a +target with no extension (`RUN post-invoice`) and a path with no extension. The two rules +split one population and no `RUN` is reported twice. (The other choice, CL004 for all and a +suppression in `LINT0001`, changes the default output, which must not change.) + +The rest of the rows are choices that the corpus forced. The first version reported every bare +name that nothing on the search path supplies: **586 of 601 findings in the second corpus were +procedures that another file defines**, called through a super procedure or a persistent +procedure with a plain `RUN name`. + +| # | The model | Confidence | +|---|---|---| +| R1 | A plain `RUN name` finds, in order, an internal procedure, a super procedure's procedure and a program. The model sees the first and the last. | Documented. | +| R2 | A bare name is reported only when the index lists the files of the workspace and **no file defines a `PROCEDURE` of that name**, in its own text or in an include file that a listed file names (an include is followed when its name is written out and resolves in the file's directory or on the include paths; `{&name}` and `{1}` are not followed). The files are read on the first bare name that misses, once per run. An index with no list (the language server) reports no bare name. | Proposed. | +| R3 | In an include checked alone, and in a file with an include that did not arrive, a bare name may be a procedure of the includer: not reported. | Proposed (as TX004). | +| R4 | In a file that binds a web service port type (`RUN port-type SET h ON SERVER s`, without `PERSISTENT`), a bare name is an operation of the service: not reported. | Inferred (one case in the second corpus; the operation names are in a WSDL). | +| R5 | A path into a directory the OpenEdge install ships (`adecomm/`, `adeshar/`, `prodict/`, ...) is not reported: a workspace that does not configure the install cannot supply it. | Proposed. | +| R6 | A target that starts with `..` is relative to a directory the search paths do not cover (the rule `LINT0001` already keeps): not reported. | Inferred (five cases in the second corpus: the program is there, one directory up from where the search starts). | +| R7 | The parser reads a name up to a digit after a slash and stops (`RUN dir/870-recon.p` is `dir/870`). A name followed at once by a letter, a digit, `-`, `_`, `/` or `.x` is not reported. | Verified (one case in the first corpus). | + +### 10.5 The rules (M6, first slice) + +Both are preview `strict.calls` rules, Warning, without an autofix, context-free and routed like +TX004 (reported in the include when the include alone shows it, else at the `{...}` site of the +includer). With the group off nothing is computed: the rules do not run, the index reads no file +for a list or a procedure name, and `RunShared::calls` is not built. + +- **CL003** reports, per `RUN` of a resolved target: a count that differs (one finding, on the + target); then, with the counts equal and per argument, at most one finding: the kind (P5), the + mode (P3), an expression as `OUTPUT` (P4), a handle against a type it does not convert to + (P7). It is silent for a callee that is Unknown, an argument of unknown type, a call whose + argument list holds an include reference or a gap the preprocessor left, a recovered parse, + and, in an include checked alone or in a file with a lost include, a bare name. +- **CL004** reports a static `RUN` of a name that nothing can run (section 10.4). + +### 10.6 Corpus check + +The two corpora of section 7.6, with their schemas, include paths and workspace sources, and the +two rules switched on in memory (`crates/oxabl/examples/calls_corpus.rs`; like the other corpus +examples it counts per top directory, `OXABL_CALLS_FINDINGS=` writes every finding, and +`OXABL_CALLS_ONLY=` checks the files whose path holds the text). No panic. + +| | First corpus | Second corpus | +|---|---|---| +| Roots, include files | 10,619 and 3,549 | 2,526 and 782 | +| CL003 | 163 (157 count, 3 mode, 3 kind) | 25 (24 count, 1 mode) | +| CL004 | 16 | 9 | +| At a use site (CL003, CL004) | 3, 0 | 3, 0 | +| Top directories with a finding (at most in one), CL003 and CL004 | 29 (28) and 4 (12) | 4 (14) and 3 (3) | + +**Default output.** With the group off, `oxabl check --json --no-format .` is byte-identical to +the base (the commit this branch grew from) on both trees. +**Cost.** With the group off nothing is computed. On the first corpus, three alternating runs of +`oxabl check --json --no-format .` take 3.97 s to 4.21 s of wall time (77 s to 82 s of CPU) for the +base and 4.01 s to 4.23 s (77 s to 81 s) for this branch. With the two rules on, in one process +(`calls_corpus`, one thread), the first corpus takes 103.6 s against 57.0 s with them off, and the +second 9.0 s against 5.7 s. The extra work is the read, expansion and parse of each callee file for +its parameter list, and one pass over every file and include of the run for procedure names, made +when the first bare name misses. + +**Triage.** "Right" means that the finding says what the code does under the rules above and the +configured search paths, not that the code fails at run time. Many right CL003 findings are a +callee that gained a parameter its callers do not pass (a trailing `INTEGER`, a class object, a +`TABLE`), or a caller that dropped the `{globals-out.i}` the others pass. + +| Round | Wrong | What was wrong, and the change | +|---|---|---| +| 1, first model (the full lists) | CL004: 586 of 601 findings in the second corpus; CL003: 1,481 findings in the first corpus, of which the later model keeps 163 | CL004: a bare name that a super or persistent procedure defines (R2). CL003: `INPUT BUFFER b:HANDLE` read as a buffer (366); `INTEGER` to `CHARACTER` and other pairs that convert (P7: 470 type findings, 340 of that one kind); a procedure whose parameters sit in a `DO:` block read as having none (P10, about 420 count findings); a program that is empty or all comment read as having none | +| 2, after those | CL003: 219 count findings before the include rule of 10.3, 157 after; CL004 2 of 14 and 1 of 17 | An include-supplied list read in another environment (10.3). `adecomm/` (R5), a web service operation (R4), `..` paths (R6), a name read short (R7) | +| 3, every finding of the second corpus (34); the first corpus: 15 CL003 findings spread over the sorted list, and all 16 CL004 | 0 of 34, 0 of 31 | | + +That is 0 wrong in the 65 findings read in round 3. The sample is small, so read the +false-positive rate as under 5 %. Right findings are of five kinds: a count that differs because +a callee gained or lost a parameter (most of the CL003 findings read); `RUN x IN h` of a +persistent program whose procedure changed; a caller that passes the parameters in another order +after a change (the `TABLE` and scalar kinds); an `OUTPUT` parameter passed without `OUTPUT`; a +bare name that nothing defines (a typo, a procedure that was removed, a class that calls +procedures of a template that no longer exists). Some right findings depend on the configured +search path: in the second corpus, callers in one directory call two programs that exist in two +directories with different lists, and only one of the directories is on the configured path.