diff --git a/crates/pgls_analyser/tests/postgres_regress.rs b/crates/pgls_analyser/tests/postgres_regress.rs index 9f2bb0bad..7a27cee37 100644 --- a/crates/pgls_analyser/tests/postgres_regress.rs +++ b/crates/pgls_analyser/tests/postgres_regress.rs @@ -4,6 +4,10 @@ //! A finding on a statement Postgres accepted is a false positive and fails the test. Findings on //! statements Postgres rejected are snapshotted per version, so a change in what we detect shows //! up in review. +//! +//! The body of a `LANGUAGE sql` function is analysed like the workspace does it: right after the +//! `CREATE FUNCTION`, with the function's parameters in scope. A finding in the body of a function +//! Postgres accepted is a false positive too. use std::{collections::BTreeMap, sync::Arc}; @@ -12,10 +16,12 @@ use pgls_analyser::{ AnalysableStatement, Analyser, AnalyserConfig, AnalyserParams, LinterOptions, TYPECHECK_GROUP, UnparsableStatement, }; -use pgls_catalog::{CatalogBase, Snapshot}; +use pgls_catalog::{CatalogBase, Snapshot, resolve::FunctionContext}; use pgls_console::StdDisplay; use pgls_diagnostics::{Diagnostic, PrintDiagnostic}; use pgls_postgres_regress::{File, Verdict, Version}; +use pgls_query::NodeEnum; +use pgls_text_size::TextRange; use rayon::prelude::*; /// Findings on statements Postgres accepted only because an earlier statement of the file failed. @@ -26,39 +32,72 @@ const KNOWN_ARTEFACTS: &[(&str, &str, &str)] = &[( "`DROP TABLE writetest` fails in a read-only session, so the table still exists", )]; +/// Known false positives in the bodies of SQL functions Postgres accepted: the file, a snippet of +/// the function, the rule, and the root cause. Remove an entry once it is fixed; an entry that +/// matches nothing fails the test. +const KNOWN_FALSE_POSITIVES: &[(&str, &str, &str, &str)] = &[( + "polymorphism", + "select array_prepend($1, $2)", + "functionArgumentMismatch", + "parameters of polymorphic types keep the pseudo-type in the body, so `$1 anyelement` doesn't \ + match `anycompatible`", +)]; + #[test] fn typecheck_reports_nothing_postgres_accepts() { let mut false_positives = Vec::new(); + let mut fixed = vec![true; KNOWN_FALSE_POSITIVES.len()]; for version in pgls_postgres_regress::versions() { let catalog = catalog(&version); let files = version.files(); - let findings = files + let checked = files .par_iter() - .flat_map_iter(|file| check(&catalog, file)) + .map(|file| check(&catalog, file)) .collect::>(); - let analysed = files.iter().map(|x| x.statements.len()).sum::(); + let analysed = Analysed { + statements: files.iter().map(|x| x.statements.len()).sum(), + bodies: checked.iter().map(|x| x.bodies).sum(), + }; let mut rejected = Vec::new(); let mut artefacts = Vec::new(); - for finding in findings { + let mut known_false_positives = Vec::new(); + for finding in checked.into_iter().flat_map(|x| x.findings) { match &finding.verdict { - Verdict::Accepted => match known_artefact(&finding) { - Some(reason) => artefacts.push(format!("{}\n {reason}", finding.location())), - None => false_positives.push(format!( - "{}/{}: {}\n {}\n {}", - version.major, - finding.location(), - finding.rule, - finding.message, - truncate(&finding.sql, 300) - )), - }, + Verdict::Accepted => { + if let Some(reason) = known_artefact(&finding) { + artefacts.push(format!("{}\n {reason}", finding.location())); + } else if let Some(index) = known_false_positive(&finding) { + fixed[index] = false; + let (_, _, _, reason) = KNOWN_FALSE_POSITIVES[index]; + known_false_positives.push(format!( + "{} {}\n {reason}", + finding.location(), + finding.rule + )); + } else { + false_positives.push(format!( + "{}/{}: {}\n {}\n {}", + version.major, + finding.location(), + finding.rule, + finding.message, + truncate(&finding.sql, 300) + )); + } + } Verdict::Rejected(state) => rejected.push((state.clone(), finding)), Verdict::Skipped => {} } } insta::assert_snapshot!( format!("typecheck_{}", version.major), - report(&version, analysed, &rejected, &artefacts) + report( + &version, + &analysed, + &rejected, + &artefacts, + &known_false_positives + ) ); } assert!( @@ -67,6 +106,28 @@ fn typecheck_reports_nothing_postgres_accepts() { false_positives.len(), false_positives.join("\n\n") ); + let fixed = KNOWN_FALSE_POSITIVES + .iter() + .zip(fixed) + .filter(|(_, fixed)| *fixed) + .map(|((file, needle, rule, _), _)| format!("{file} {rule} `{needle}`")) + .collect::>(); + assert!( + fixed.is_empty(), + "known false positives that no longer occur, remove them from KNOWN_FALSE_POSITIVES:\n{}", + fixed.join("\n") + ); +} + +struct Analysed { + statements: usize, + /// The SQL function bodies analysed. + bodies: usize, +} + +struct Checked { + findings: Vec, + bodies: usize, } struct Finding { @@ -74,6 +135,8 @@ struct Finding { /// Where the statement starts. line: usize, col: usize, + /// Whether the finding is in the body of a SQL function. + in_body: bool, rule: String, message: String, verdict: Verdict, @@ -82,7 +145,8 @@ struct Finding { impl Finding { fn location(&self) -> String { - format!("{}.sql:{}:{}", self.file, self.line, self.col) + let body = if self.in_body { " (body)" } else { "" }; + format!("{}.sql:{}:{}{body}", self.file, self.line, self.col) } } @@ -94,7 +158,7 @@ fn catalog(version: &Version) -> Arc { /// Lints a file like the workspace does: statements our parser doesn't understand (newer syntax) /// are passed on as unparsable. -fn check(catalog: &Arc, file: &File) -> Vec { +fn check(catalog: &Arc, file: &File) -> Checked { let options = LinterOptions::default(); let rules = [RuleFilter::Group(TYPECHECK_GROUP)]; let analyser = Analyser::new(AnalyserConfig { @@ -103,17 +167,24 @@ fn check(catalog: &Arc, file: &File) -> Vec { }); let mut stmts = Vec::new(); let mut unparsable = Vec::new(); + let mut bodies = Vec::new(); for statement in &file.statements { let sql = file.sql(statement); - // psql variables (`:name`, `:'name'`) become positional parameters, as in the workspace. - let conversion = pgls_lexer::convert_to_positional_params_with_metadata(sql); - match pgls_query::parse(&conversion.sql) { - Ok(ast) => stmts.extend(ast.into_root().map(|root| { - AnalysableStatement::new(root, statement.range) - .with_sql(sql) - .with_identifier_parameters(conversion.has_identifier_parameters) - })), - Err(_) => unparsable.push(UnparsableStatement { + match parse(sql) { + Ok(Some((root, has_identifier_parameters))) => { + let body = sql_function_body(&root, sql, statement.range); + stmts.push( + AnalysableStatement::new(root, statement.range) + .with_sql(sql) + .with_identifier_parameters(has_identifier_parameters), + ); + if let Some(body) = body { + bodies.push(body.range); + stmts.push(body); + } + } + Ok(None) => {} + Err(()) => unparsable.push(UnparsableStatement { range: statement.range, sql: sql.to_owned(), }), @@ -127,7 +198,7 @@ fn check(catalog: &Arc, file: &File) -> Vec { typecheck: true, ..Default::default() }); - diagnostics + let findings = diagnostics .iter() .map(|diagnostic| { let span = diagnostic.location().span.expect("findings have a span"); @@ -141,6 +212,9 @@ fn check(catalog: &Arc, file: &File) -> Vec { file: file.name.clone(), line, col, + in_body: bodies + .iter() + .any(|body| body.contains_inclusive(span.start())), rule: diagnostic .get_category_name() .trim_start_matches("lint/") @@ -151,7 +225,49 @@ fn check(catalog: &Arc, file: &File) -> Vec { sql: file.sql(statement).to_owned(), } }) - .collect() + .collect(); + Checked { + findings, + bodies: bodies.len(), + } +} + +/// The root of a statement, and whether it has psql identifier parameters. `Err` if it doesn't +/// parse, `None` if it isn't a single statement. +fn parse(sql: &str) -> Result, ()> { + // psql variables (`:name`, `:'name'`) become positional parameters, as in the workspace. + let conversion = pgls_lexer::convert_to_positional_params_with_metadata(sql); + let ast = pgls_query::parse(&conversion.sql).map_err(|_| ())?; + Ok(ast + .into_root() + .map(|root| (root, conversion.has_identifier_parameters))) +} + +/// The statement in the body of a `LANGUAGE sql` function, as the workspace extracts and analyses +/// it. +fn sql_function_body(root: &NodeEnum, sql: &str, range: TextRange) -> Option { + let NodeEnum::CreateFunctionStmt(create_fn) = root else { + return None; + }; + let body = pgls_query_ext::utils::sql_function_body(root, sql)?; + let (body_root, has_identifier_parameters) = parse(&body.body).ok()??; + Some( + AnalysableStatement::new(body_root, body.range + range.start()) + .with_sql(body.body) + .with_function(FunctionContext::from_create_function(create_fn)) + .with_identifier_parameters(has_identifier_parameters), + ) +} + +fn known_false_positive(finding: &Finding) -> Option { + KNOWN_FALSE_POSITIVES + .iter() + .position(|(file, needle, rule, _)| { + finding.in_body + && finding.file == *file + && finding.rule == *rule + && finding.sql.contains(needle) + }) } fn known_artefact(finding: &Finding) -> Option<&'static str> { @@ -161,20 +277,24 @@ fn known_artefact(finding: &Finding) -> Option<&'static str> { .map(|(_, _, reason)| *reason) } -/// A summary per rule and SQLSTATE, then one line per finding on a rejected statement. +/// A summary per rule and SQLSTATE, the known artefacts and false positives, then one line per +/// finding on a rejected statement. fn report( version: &Version, - analysed: usize, + analysed: &Analysed, rejected: &[(String, Finding)], artefacts: &[String], + false_positives: &[String], ) -> String { let mut counts: BTreeMap<(&str, &str), usize> = BTreeMap::new(); for (state, finding) in rejected { *counts.entry((&finding.rule, state)).or_default() += 1; } let mut report = format!( - "{}: {analysed} statements, {} findings on statements Postgres rejected\n\n", + "{}: {} statements and {} SQL function bodies, {} findings on statements Postgres rejected\n\n", version.tag, + analysed.statements, + analysed.bodies, rejected.len() ); for ((rule, state), count) in counts { @@ -186,6 +306,12 @@ fn report( report.push_str(&format!("{artefact}\n")); } } + if !false_positives.is_empty() { + report.push_str("\nKnown false positives\n"); + for false_positive in false_positives { + report.push_str(&format!("{false_positive}\n")); + } + } report.push_str("\nFindings\n"); let mut findings = rejected.iter().collect::>(); findings.sort_by(|(_, a), (_, b)| { diff --git a/crates/pgls_analyser/tests/rules_tests.rs b/crates/pgls_analyser/tests/rules_tests.rs index a0ccf87a7..9b185cf3c 100644 --- a/crates/pgls_analyser/tests/rules_tests.rs +++ b/crates/pgls_analyser/tests/rules_tests.rs @@ -116,19 +116,19 @@ fn rule_test(full_path: &'static str, _: &str, _: &str) { let split = pgls_statement_splitter::split(&query); - // Statements that don't parse are passed on as such, like the workspace does. + // Statements that don't parse are passed on as such, like the workspace does. The body of a + // SQL function follows the function, with its parameters in scope. let mut stmts = Vec::new(); let mut unparsable = Vec::new(); for r in &split.ranges { let text = &query[*r]; match pgls_query::parse(text) { - Ok(ast) => stmts.push( - AnalysableStatement::new( - ast.into_root().expect("Failed to convert AST to root node"), - *r, - ) - .with_sql(text), - ), + Ok(ast) => { + let root = ast.into_root().expect("Failed to convert AST to root node"); + let body = sql_function_body(&root, text, *r); + stmts.push(AnalysableStatement::new(root, *r).with_sql(text)); + stmts.extend(body); + } Err(_) => unparsable.push(UnparsableStatement { range: *r, sql: text.to_owned(), @@ -162,6 +162,24 @@ fn rule_test(full_path: &'static str, _: &str, _: &str) { expectation.assert(results.as_slice()); } +/// The statement in the body of a `LANGUAGE sql` function, as the workspace extracts it. +fn sql_function_body( + root: &pgls_query::NodeEnum, + sql: &str, + range: TextRange, +) -> Option { + let pgls_query::NodeEnum::CreateFunctionStmt(create_fn) = root else { + return None; + }; + let body = pgls_query_ext::utils::sql_function_body(root, sql)?; + let body_root = pgls_query::parse(&body.body).ok()?.into_root()?; + Some( + AnalysableStatement::new(body_root, body.range + range.start()) + .with_sql(body.body) + .with_function(pgls_catalog::resolve::FunctionContext::from_create_function(create_fn)), + ) +} + /// The migration sections of the file, each starting at a `-- section: up` or /// `-- section: down transaction:false` line. fn migration_sections(query: &str) -> Vec { diff --git a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_15.snap b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_15.snap index 2859eb089..8d3ab1b8b 100644 --- a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_15.snap +++ b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_15.snap @@ -1,8 +1,8 @@ --- source: crates/pgls_analyser/tests/postgres_regress.rs -expression: "report(&version, analysed, &rejected, &artefacts)" +expression: "report(&version, &analysed, &rejected, &artefacts, &known_false_positives)" --- -REL_15_19: 39967 statements, 1583 findings on statements Postgres rejected +REL_15_19: 39967 statements and 347 SQL function bodies, 1586 findings on statements Postgres rejected ambiguousColumn 42702: 6 functionArgumentMismatch 42725: 5 @@ -21,7 +21,7 @@ unknownFunction 42P01: 18 unknownRelation 0A000: 2 unknownRelation 25006: 2 unknownRelation 42501: 1 -unknownRelation 42P01: 1284 +unknownRelation 42P01: 1287 unknownType 42704: 88 unknownType 42P01: 2 @@ -31,6 +31,10 @@ transactions.sql:90:1 transactions.sql:92:1 `DROP TABLE writetest` fails in a read-only session, so the table still exists +Known false positives +polymorphism.sql:690:1 (body) functionArgumentMismatch + parameters of polymorphic types keep the pseudo-type in the body, so `$1 anyelement` doesn't match `anycompatible` + Findings aggregates.sql:23:1 42P01 unknownRelation aggregates.sql:32:1 42P01 unknownRelation @@ -1239,6 +1243,7 @@ select_implicit.sql:129:1 42702 ambiguousColumn select_implicit.sql:140:1 42702 ambiguousColumn select_into.sql:5:1 42P01 unknownRelation select_into.sql:12:1 42P01 unknownRelation +select_into.sql:90:1 (body) 42P01 unknownRelation select_into.sql:97:1 42P01 unknownRelation select_parallel.sql:17:1 42P01 unknownRelation select_parallel.sql:19:1 42P01 unknownRelation @@ -1557,6 +1562,8 @@ window.sql:381:1 42P01 unknownRelation window.sql:385:1 42P01 unknownRelation window.sql:389:1 42P01 unknownRelation window.sql:393:1 42P01 unknownRelation +window.sql:514:1 (body) 42P01 unknownRelation +window.sql:534:1 (body) 42P01 unknownRelation window.sql:552:1 42P01 unknownRelation window.sql:556:1 42P01 unknownRelation window.sql:560:1 42P01 unknownRelation diff --git a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_16.snap b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_16.snap index 5bb868c8b..491b5ae2e 100644 --- a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_16.snap +++ b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_16.snap @@ -1,8 +1,8 @@ --- source: crates/pgls_analyser/tests/postgres_regress.rs -expression: "report(&version, analysed, &rejected, &artefacts)" +expression: "report(&version, &analysed, &rejected, &artefacts, &known_false_positives)" --- -REL_16_15: 41492 statements, 1692 findings on statements Postgres rejected +REL_16_15: 41492 statements and 350 SQL function bodies, 1696 findings on statements Postgres rejected ambiguousColumn 42702: 6 functionArgumentMismatch 42725: 5 @@ -16,12 +16,12 @@ operatorTypeMismatch 42883: 9 unknownColumn 42703: 38 unknownColumn 42P01: 3 unknownFunction 42704: 4 -unknownFunction 42883: 96 +unknownFunction 42883: 97 unknownFunction 42P01: 19 unknownRelation 0A000: 2 unknownRelation 25006: 2 unknownRelation 42501: 1 -unknownRelation 42P01: 1386 +unknownRelation 42P01: 1389 unknownType 42704: 88 unknownType 42P01: 2 @@ -31,6 +31,10 @@ transactions.sql:108:1 transactions.sql:110:1 `DROP TABLE writetest` fails in a read-only session, so the table still exists +Known false positives +polymorphism.sql:690:1 (body) functionArgumentMismatch + parameters of polymorphic types keep the pseudo-type in the body, so `$1 anyelement` doesn't match `anycompatible` + Findings aggregates.sql:23:1 42P01 unknownRelation aggregates.sql:37:1 42P01 unknownRelation @@ -230,6 +234,7 @@ brin_multi.sql:48:1 42P01 unknownRelation char.sql:71:1 42P01 unknownRelation char.sql:73:1 42P01 unknownRelation cluster.sql:253:1 42P01 unknownRelation +compression.sql:50:1 (body) 42883 unknownFunction compression.sql:138:1 42883 unknownFunction create_am.sql:49:1 42P01 unknownRelation create_am.sql:53:1 42P01 unknownRelation @@ -1330,6 +1335,7 @@ select_implicit.sql:129:1 42702 ambiguousColumn select_implicit.sql:140:1 42702 ambiguousColumn select_into.sql:5:1 42P01 unknownRelation select_into.sql:12:1 42P01 unknownRelation +select_into.sql:90:1 (body) 42P01 unknownRelation select_into.sql:97:1 42P01 unknownRelation select_parallel.sql:17:1 42P01 unknownRelation select_parallel.sql:19:1 42P01 unknownRelation @@ -1666,6 +1672,8 @@ window.sql:385:1 42P01 unknownRelation window.sql:389:1 42P01 unknownRelation window.sql:393:1 42P01 unknownRelation window.sql:397:1 42P01 unknownRelation +window.sql:518:1 (body) 42P01 unknownRelation +window.sql:538:1 (body) 42P01 unknownRelation window.sql:556:1 42P01 unknownRelation window.sql:560:1 42P01 unknownRelation window.sql:564:1 42P01 unknownRelation diff --git a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_17.snap b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_17.snap index 5392cde97..51472764d 100644 --- a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_17.snap +++ b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_17.snap @@ -1,8 +1,8 @@ --- source: crates/pgls_analyser/tests/postgres_regress.rs -expression: "report(&version, analysed, &rejected, &artefacts)" +expression: "report(&version, &analysed, &rejected, &artefacts, &known_false_positives)" --- -REL_17_11: 43838 statements, 1747 findings on statements Postgres rejected +REL_17_11: 43838 statements and 355 SQL function bodies, 1751 findings on statements Postgres rejected ambiguousColumn 42702: 6 functionArgumentMismatch 42725: 5 @@ -16,12 +16,12 @@ operatorTypeMismatch 42883: 9 unknownColumn 42703: 40 unknownColumn 42P01: 3 unknownFunction 42704: 4 -unknownFunction 42883: 96 +unknownFunction 42883: 97 unknownFunction 42P01: 19 unknownRelation 0A000: 2 unknownRelation 25006: 2 unknownRelation 42501: 1 -unknownRelation 42P01: 1439 +unknownRelation 42P01: 1442 unknownType 42704: 88 unknownType 42P01: 2 @@ -31,6 +31,10 @@ transactions.sql:108:1 transactions.sql:110:1 `DROP TABLE writetest` fails in a read-only session, so the table still exists +Known false positives +polymorphism.sql:690:1 (body) functionArgumentMismatch + parameters of polymorphic types keep the pseudo-type in the body, so `$1 anyelement` doesn't match `anycompatible` + Findings aggregates.sql:23:1 42P01 unknownRelation aggregates.sql:37:1 42P01 unknownRelation @@ -240,6 +244,7 @@ btree_index.sql:151:1 42P01 unknownRelation char.sql:71:1 42P01 unknownRelation char.sql:73:1 42P01 unknownRelation cluster.sql:258:1 42P01 unknownRelation +compression.sql:50:1 (body) 42883 unknownFunction compression.sql:140:1 42883 unknownFunction create_am.sql:49:1 42P01 unknownRelation create_am.sql:53:1 42P01 unknownRelation @@ -1344,6 +1349,7 @@ select_implicit.sql:129:1 42702 ambiguousColumn select_implicit.sql:140:1 42702 ambiguousColumn select_into.sql:5:1 42P01 unknownRelation select_into.sql:12:1 42P01 unknownRelation +select_into.sql:90:1 (body) 42P01 unknownRelation select_into.sql:97:1 42P01 unknownRelation select_parallel.sql:17:1 42P01 unknownRelation select_parallel.sql:19:1 42P01 unknownRelation @@ -1707,6 +1713,8 @@ window.sql:385:1 42P01 unknownRelation window.sql:389:1 42P01 unknownRelation window.sql:393:1 42P01 unknownRelation window.sql:397:1 42P01 unknownRelation +window.sql:518:1 (body) 42P01 unknownRelation +window.sql:538:1 (body) 42P01 unknownRelation window.sql:556:1 42P01 unknownRelation window.sql:560:1 42P01 unknownRelation window.sql:564:1 42P01 unknownRelation diff --git a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_18.snap b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_18.snap index 63759347e..fbf2ea265 100644 --- a/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_18.snap +++ b/crates/pgls_analyser/tests/snapshots/postgres_regress__typecheck_18.snap @@ -1,8 +1,8 @@ --- source: crates/pgls_analyser/tests/postgres_regress.rs -expression: "report(&version, analysed, &rejected, &artefacts)" +expression: "report(&version, &analysed, &rejected, &artefacts, &known_false_positives)" --- -REL_18_6: 47289 statements, 1840 findings on statements Postgres rejected +REL_18_6: 47289 statements and 359 SQL function bodies, 1844 findings on statements Postgres rejected ambiguousColumn 42702: 6 functionArgumentMismatch 42725: 5 @@ -16,12 +16,12 @@ operatorTypeMismatch 42883: 9 unknownColumn 42703: 40 unknownColumn 42P01: 3 unknownFunction 42704: 4 -unknownFunction 42883: 95 +unknownFunction 42883: 96 unknownFunction 42P01: 19 unknownRelation 0A000: 2 unknownRelation 25006: 2 unknownRelation 42501: 1 -unknownRelation 42P01: 1533 +unknownRelation 42P01: 1536 unknownType 42704: 88 unknownType 42P01: 2 @@ -31,6 +31,10 @@ transactions.sql:108:1 transactions.sql:110:1 `DROP TABLE writetest` fails in a read-only session, so the table still exists +Known false positives +polymorphism.sql:690:1 (body) functionArgumentMismatch + parameters of polymorphic types keep the pseudo-type in the body, so `$1 anyelement` doesn't match `anycompatible` + Findings aggregates.sql:23:1 42P01 unknownRelation aggregates.sql:37:1 42P01 unknownRelation @@ -240,6 +244,7 @@ btree_index.sql:275:1 42P01 unknownRelation char.sql:71:1 42P01 unknownRelation char.sql:73:1 42P01 unknownRelation cluster.sql:258:1 42P01 unknownRelation +compression.sql:50:1 (body) 42883 unknownFunction compression.sql:140:1 42883 unknownFunction create_am.sql:49:1 42P01 unknownRelation create_am.sql:53:1 42P01 unknownRelation @@ -1353,6 +1358,7 @@ select_implicit.sql:129:1 42702 ambiguousColumn select_implicit.sql:140:1 42702 ambiguousColumn select_into.sql:5:1 42P01 unknownRelation select_into.sql:12:1 42P01 unknownRelation +select_into.sql:90:1 (body) 42P01 unknownRelation select_into.sql:97:1 42P01 unknownRelation select_parallel.sql:24:1 42P01 unknownRelation select_parallel.sql:26:1 42P01 unknownRelation @@ -1800,6 +1806,8 @@ window.sql:385:1 42P01 unknownRelation window.sql:389:1 42P01 unknownRelation window.sql:393:1 42P01 unknownRelation window.sql:397:1 42P01 unknownRelation +window.sql:518:1 (body) 42P01 unknownRelation +window.sql:538:1 (body) 42P01 unknownRelation window.sql:556:1 42P01 unknownRelation window.sql:560:1 42P01 unknownRelation window.sql:564:1 42P01 unknownRelation diff --git a/crates/pgls_catalog/src/resolve/nodes/create_function_stmt.rs b/crates/pgls_catalog/src/resolve/nodes/create_function_stmt.rs index 81929c47e..e022033c5 100644 --- a/crates/pgls_catalog/src/resolve/nodes/create_function_stmt.rs +++ b/crates/pgls_catalog/src/resolve/nodes/create_function_stmt.rs @@ -132,7 +132,7 @@ pub(super) fn resolve_create_function_stmt(r: &mut Resolver, n: &CreateFunctionS let Some(shape) = shape else { return; }; - let Some(found) = output_columns(r, n, ¶ms, last) else { + let Some(found) = output_columns(r, n, last) else { return; }; if let Some(mismatch) = compare(r, &shape, &found) { @@ -211,6 +211,40 @@ fn compare(r: &Resolver, shape: &Shape, found: &[Option]) -> Option Option { + let mut params = Vec::new(); + for param in &n.parameters { + let Some(NodeEnum::FunctionParameter(param)) = ¶m.node else { + return None; + }; + if matches!(param.mode(), Mode::FuncParamOut | Mode::FuncParamTable) { + continue; + } + let type_name = param.arg_type.as_ref()?; + let mut names = string_values(&type_name.names)?; + params.push(FunctionParam { + name: (!param.name.is_empty()).then(|| param.name.clone()), + type_name: names.pop()?, + type_schema: names.pop(), + is_array: !type_name.array_bounds.is_empty(), + }); + } + Some(FunctionContext { + function_name: string_values(&n.funcname)?.pop()?, + params, + }) + } +} + fn is_sql_function(n: &CreateFunctionStmt) -> bool { // `BEGIN ATOMIC` and `RETURN` bodies only exist for SQL functions. n.sql_body.is_some() @@ -402,26 +436,9 @@ fn type_id(r: &Resolver, name: &str) -> Option { fn output_columns( r: &Resolver, n: &CreateFunctionStmt, - params: &[&FunctionParameter], last: &NodeEnum, ) -> Option>> { - let function = FunctionContext { - function_name: string_values(&n.funcname)?.pop()?, - params: params - .iter() - .filter(|param| !matches!(param.mode(), Mode::FuncParamOut | Mode::FuncParamTable)) - .map(|param| { - let type_name = param.arg_type.as_ref()?; - let mut names = string_values(&type_name.names)?; - Some(FunctionParam { - name: (!param.name.is_empty()).then(|| param.name.clone()), - type_name: names.pop()?, - type_schema: names.pop(), - is_array: !type_name.array_bounds.is_empty(), - }) - }) - .collect::>()?, - }; + let function = FunctionContext::from_create_function(n)?; // The body is resolved on its own, and its findings are reported for its statements. let mut body = Resolver::new(r.catalog, r.search_path, Some(&function), None); let columns = resolve_node_enum(&mut body, last)?; diff --git a/crates/pgls_catalog/src/resolve/tests.rs b/crates/pgls_catalog/src/resolve/tests.rs index a26288dc7..86a9aa7b1 100644 --- a/crates/pgls_catalog/src/resolve/tests.rs +++ b/crates/pgls_catalog/src/resolve/tests.rs @@ -490,6 +490,53 @@ fn unknown_objects_are_silent() { ); } +#[test] +fn function_context_has_the_input_parameters() { + let context = |sql: &str| { + let Some(NodeEnum::CreateFunctionStmt(n)) = pgls_query::parse(sql).unwrap().into_root() + else { + panic!("not a CREATE FUNCTION: {sql}"); + }; + FunctionContext::from_create_function(&n).unwrap() + }; + let param = + |name: Option<&str>, type_schema: Option<&str>, type_name: &str, is_array| FunctionParam { + name: name.map(Into::into), + type_schema: type_schema.map(Into::into), + type_name: type_name.into(), + is_array, + }; + + let add = context( + "create function public.add(a integer[], int) returns int language sql as 'select 1'", + ); + assert_eq!(add.function_name, "add"); + assert_eq!( + add.params, + [ + param(Some("a"), Some("pg_catalog"), "int4", true), + param(None, Some("pg_catalog"), "int4", false), + ] + ); + // `OUT` and `RETURNS TABLE` columns are not in scope, and don't count for `$n`. + assert_eq!( + context("create function f(p bigint) returns table(id bigint) language sql as 'select 1'") + .params, + [param(Some("p"), Some("pg_catalog"), "int8", false)] + ); + assert_eq!( + context( + "create function f(out x int, a text, inout b int, variadic c int[]) language sql as 'select 1, 2'" + ) + .params, + [ + param(Some("a"), None, "text", false), + param(Some("b"), Some("pg_catalog"), "int4", false), + param(Some("c"), Some("pg_catalog"), "int4", true), + ] + ); +} + #[test] fn function_bodies() { let function = FunctionContext { diff --git a/crates/pgls_query_ext/src/utils.rs b/crates/pgls_query_ext/src/utils.rs index abe047764..62f874ba3 100644 --- a/crates/pgls_query_ext/src/utils.rs +++ b/crates/pgls_query_ext/src/utils.rs @@ -1,3 +1,5 @@ +use pgls_text_size::{TextRange, TextSize}; + /// Helper function to find a specific option value from function options pub fn find_option_value( create_fn: &pgls_query::protobuf::CreateFunctionStmt, @@ -40,6 +42,33 @@ pub fn find_option_value( }) } +/// The body of a `LANGUAGE sql` function, written as a string (`AS '...'` or `AS $$...$$`). +#[derive(Debug, Clone)] +pub struct SqlFunctionBody { + /// Where the body is in the statement. + pub range: TextRange, + pub body: String, +} + +/// The body of `stmt` if it creates a `LANGUAGE sql` function with a string body. `None` for +/// `BEGIN ATOMIC` and `RETURN` bodies, and for bodies whose text differs from the source, like +/// `'...'` with doubled quotes. +pub fn sql_function_body(stmt: &pgls_query::NodeEnum, sql: &str) -> Option { + let pgls_query::NodeEnum::CreateFunctionStmt(create_fn) = stmt else { + return None; + }; + if find_option_value(create_fn, "language")? != "sql" { + return None; + } + let body = find_option_value(create_fn, "as")?; + let start = sql.find(&body)?; + let range = TextRange::at( + TextSize::try_from(start).ok()?, + TextSize::try_from(body.len()).ok()?, + ); + Some(SqlFunctionBody { range, body }) +} + pub fn parse_name(nodes: &[pgls_query::protobuf::Node]) -> Option<(Option, String)> { let names = nodes .iter() diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index b186db9f2..33939f5e4 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -683,7 +683,7 @@ impl WorkspaceServer { .iter() .any(|section| section.contains(range.start())) }) - .map(|(id, range, ast, cst, _)| { + .map(|(id, range, ast, cst)| { let statement = statements .iter() .find(|statement| statement.range == range) diff --git a/crates/pgls_workspace/src/workspace/server/document.rs b/crates/pgls_workspace/src/workspace/server/document.rs index 4dd5c91c5..c1bdcadf5 100644 --- a/crates/pgls_workspace/src/workspace/server/document.rs +++ b/crates/pgls_workspace/src/workspace/server/document.rs @@ -2,14 +2,13 @@ use std::sync::Arc; use pgls_analyser::{AnalysableStatement, UnparsableStatement}; use pgls_diagnostics::{Diagnostic, DiagnosticExt, serde::Diagnostic as SDiagnostic}; -use pgls_query_ext::diagnostics::SyntaxDiagnostic; +use pgls_query_ext::{diagnostics::SyntaxDiagnostic, utils::sql_function_body}; use pgls_suppressions::Suppressions; use pgls_text_size::{TextRange, TextSize}; use super::{ annotation::AnnotationStore, pg_query::{PgQueryStore, convert_to_positional_params_with_metadata}, - sql_function::{SQLFunctionSignature, get_sql_fn_body, get_sql_fn_signature}, statement_identifier::StatementId, tree_sitter::TreeSitterStore, }; @@ -156,7 +155,7 @@ where if let Ok(ast) = self.parser.ast_db.get_or_cache_ast(&root_id).as_ref() { // Check if this is a SQL function definition with a body - if let Some(sub_statement) = get_sql_fn_body(ast, content) { + if let Some(sub_statement) = sql_function_body(ast, content) { // Add sub-statements to our pending queue self.pending_sub_statements.push(( root_id.create_child(&sub_statement.body), @@ -231,7 +230,6 @@ impl<'a> StatementMapper<'a> for TypecheckDiagnosticsMapper { TextRange, Option, Arc, - Option, ); fn map(&self, parser: &'a Document, id: StatementId, range: TextRange) -> Self::Output { @@ -244,15 +242,7 @@ impl<'a> StatementMapper<'a> for TypecheckDiagnosticsMapper { let cst_result = parser.cst_db.get_or_cache_tree(&id); - let sql_fn_sig = id.parent().and_then(|root| { - let ast_option = parser.ast_db.get_or_cache_ast(&root).as_ref().clone().ok(); - - let ast_option = ast_option.as_ref()?; - - get_sql_fn_signature(ast_option) - }); - - (id.clone(), range, ast_option, cst_result, sql_fn_sig) + (id.clone(), range, ast_option, cst_result) } } @@ -291,8 +281,10 @@ impl<'a> StatementMapper<'a> for AnalyserDiagnosticsMapper { // Statements in the body of a SQL function can reference its parameters. let function = id.parent().and_then(|root| { let parent = parser.ast_db.get_or_cache_ast(&root); - let signature = get_sql_fn_signature(parent.as_ref().as_ref().ok()?)?; - Some(function_context(signature)) + let Ok(pgls_query::NodeEnum::CreateFunctionStmt(create_fn)) = parent.as_ref() else { + return None; + }; + pgls_catalog::resolve::FunctionContext::from_create_function(create_fn) }); ( @@ -310,22 +302,6 @@ impl<'a> StatementMapper<'a> for AnalyserDiagnosticsMapper { ) } } -fn function_context(signature: SQLFunctionSignature) -> pgls_catalog::resolve::FunctionContext { - pgls_catalog::resolve::FunctionContext { - function_name: signature.name, - params: signature - .args - .into_iter() - .map(|arg| pgls_catalog::resolve::FunctionParam { - name: arg.name, - type_schema: arg.type_.schema, - type_name: arg.type_.name, - is_array: arg.type_.is_array, - }) - .collect(), - } -} - pub struct WithCSTMapper; impl<'a> StatementMapper<'a> for WithCSTMapper { type Output = (StatementId, TextRange, Arc); @@ -467,6 +443,30 @@ mod tests { assert_eq!(stmts[1].2, "select $1 + $2;"); } + #[test] + fn sql_function_body_sees_the_parameters() { + let input = + "CREATE FUNCTION add(a int, b int) RETURNS int AS 'SELECT a + $2;' LANGUAGE sql;"; + let d = Document::new(input.to_string(), 1); + + let results = d.iter(AnalyserDiagnosticsMapper).collect::>(); + + assert_eq!(results.len(), 2); + assert!(results[0].0.as_ref().unwrap().function.is_none()); + let body = results[1].0.as_ref().unwrap(); + assert_eq!(body.sql.as_deref(), Some("SELECT a + $2;")); + let function = body.function.as_ref().unwrap(); + assert_eq!(function.function_name, "add"); + assert_eq!( + function + .params + .iter() + .map(|param| param.name.as_deref()) + .collect::>(), + [Some("a"), Some("b")] + ); + } + #[test] fn test_sync_diagnostics_mapper_plpgsql_syntax_error() { let input = " @@ -610,14 +610,11 @@ $$ LANGUAGE plpgsql;"; let results = d.iter(TypecheckDiagnosticsMapper).collect::>(); assert_eq!(results.len(), 1); - let (_id, _range, ast, cst, sql_fn_sig) = &results[0]; + let (_id, _range, ast, cst) = &results[0]; // Should have both AST and CST assert!(ast.is_some()); assert_eq!(cst.root_node().kind(), "program"); - - // Should not have SQL function signature for top-level statement - assert!(sql_fn_sig.is_none()); } #[test] @@ -630,16 +627,9 @@ $$ LANGUAGE plpgsql;"; assert_eq!(results.len(), 2); // Check the function body - let (_id, _range, ast, _cst, sql_fn_sig) = &results[1]; + let (_id, _range, ast, _cst) = &results[1]; assert_eq!(_id.content(), "SELECT $1 + $2;"); assert!(ast.is_some()); - assert!(sql_fn_sig.is_some()); - - let sig = sql_fn_sig.as_ref().unwrap(); - assert_eq!(sig.name, "add"); - assert_eq!(sig.args.len(), 2); - assert_eq!(sig.args[0].name, Some("a".to_string())); - assert_eq!(sig.args[1].name, Some("b".to_string())); } #[test] diff --git a/crates/pgls_workspace/src/workspace/server/sql_function.rs b/crates/pgls_workspace/src/workspace/server/sql_function.rs index 02b9ff36c..b8ae3645c 100644 --- a/crates/pgls_workspace/src/workspace/server/sql_function.rs +++ b/crates/pgls_workspace/src/workspace/server/sql_function.rs @@ -1,128 +1,3 @@ -use pgls_query::protobuf::FunctionParameterMode; -use pgls_text_size::TextRange; - -#[derive(Debug, Clone)] -pub struct ArgType { - pub schema: Option, - pub name: String, - pub is_array: bool, -} - -#[derive(Debug, Clone)] -pub struct SQLFunctionArg { - pub name: Option, - pub type_: ArgType, -} - -#[derive(Debug, Clone)] -pub struct SQLFunctionSignature { - #[allow(dead_code)] - pub schema: Option, - pub name: String, - pub args: Vec, -} - -#[derive(Debug, Clone)] -pub struct SQLFunctionBody { - pub range: TextRange, - pub body: String, -} - -/// Extracts the function signature from a SQL function definition -pub fn get_sql_fn_signature(ast: &pgls_query::NodeEnum) -> Option { - let create_fn = match ast { - pgls_query::NodeEnum::CreateFunctionStmt(cf) => cf, - _ => return None, - }; - - // Extract language from function options - let language = pgls_query_ext::utils::find_option_value(create_fn, "language")?; - - // Only process SQL functions - if language != "sql" { - return None; - } - - let fn_name = pgls_query_ext::utils::parse_name(&create_fn.funcname)?; - - // we return None if anything is not expected - let mut fn_args = Vec::new(); - for arg in &create_fn.parameters { - if let Some(pgls_query::NodeEnum::FunctionParameter(node)) = &arg.node { - // The body only sees the input parameters, by name and as `$n` - // ([`get_func_input_arg_names`] in [`prepare_sql_fn_parse_info`]). `OUT` and - // `RETURNS TABLE` columns only describe the result: their names are not in scope, - // and they don't count for `$n`, so `$1` in `f(OUT x int, a text)` is `a`. `INOUT` - // and `VARIADIC` parameters are inputs and stay. The position in `fn_args` is the - // `n` of `$n`. - // - // [`get_func_input_arg_names`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/utils/fmgr/funcapi.c#L1522 - // [`prepare_sql_fn_parse_info`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/executor/functions.c#L251 - if matches!( - node.mode(), - FunctionParameterMode::FuncParamOut | FunctionParameterMode::FuncParamTable - ) { - continue; - } - - let arg_name = (!node.name.is_empty()).then_some(node.name.clone()); - - let arg_type = node.arg_type.as_ref()?; - let type_name = pgls_query_ext::utils::parse_name(&arg_type.names)?; - fn_args.push(SQLFunctionArg { - name: arg_name, - type_: ArgType { - schema: type_name.0, - name: type_name.1, - is_array: node - .arg_type - .as_ref() - .map(|t| !t.array_bounds.is_empty()) - .unwrap_or(false), - }, - }); - } else { - return None; - } - } - - Some(SQLFunctionSignature { - schema: fn_name.0, - name: fn_name.1, - args: fn_args, - }) -} - -/// Extracts the SQL body from a function definition -pub fn get_sql_fn_body(ast: &pgls_query::NodeEnum, content: &str) -> Option { - let create_fn = match ast { - pgls_query::NodeEnum::CreateFunctionStmt(cf) => cf, - _ => return None, - }; - - // Extract language from function options - let language = pgls_query_ext::utils::find_option_value(create_fn, "language")?; - - // Only process SQL functions - if language != "sql" { - return None; - } - - // Extract SQL body from function options - let sql_body = pgls_query_ext::utils::find_option_value(create_fn, "as")?; - - // Find the range of the SQL body in the content - let start = content.find(&sql_body)?; - let end = start + sql_body.len(); - - let range = TextRange::new(start.try_into().unwrap(), end.try_into().unwrap()); - - Some(SQLFunctionBody { - range, - body: sql_body.clone(), - }) -} - /// Puts the formatted body into `CREATE FUNCTION ... AS ''` for a `LANGUAGE sql` function. /// /// A reformatted body goes on lines of its own between the quotes. A body that formatting leaves @@ -176,90 +51,6 @@ fn sql_fn_body_mut(ast: &mut pgls_query::NodeEnum) -> Option<&mut String> { mod tests { use super::*; - #[test] - fn sql_function_signature() { - let input = "CREATE FUNCTION add(test0 integer, test1 integer) RETURNS integer - AS 'select $1 + $2;' - LANGUAGE SQL - IMMUTABLE - RETURNS NULL ON NULL INPUT;"; - - let ast = pgls_query::parse(input).unwrap().into_root().unwrap(); - - let sig = get_sql_fn_signature(&ast); - - assert!(sig.is_some()); - - let sig = sig.unwrap(); - - let arg1 = sig.args.first().unwrap(); - - assert_eq!(arg1.name, Some("test0".to_string())); - assert_eq!(arg1.type_.name, "int4"); - - let arg2 = sig.args.get(1).unwrap(); - assert_eq!(arg2.name, Some("test1".to_string())); - assert_eq!(arg2.type_.name, "int4"); - } - - #[test] - fn output_parameters_are_not_arguments() { - let names = |input: &str| { - let ast = pgls_query::parse(input).unwrap().into_root().unwrap(); - get_sql_fn_signature(&ast) - .unwrap() - .args - .into_iter() - .map(|arg| arg.name) - .collect::>() - }; - - assert_eq!( - names( - "create function f(p_above bigint) returns table(id bigint, received_at timestamptz) language sql as 'select 1, now()'" - ), - [Some("p_above".to_string())] - ); - assert_eq!( - names( - "create function f(out x int, a text, inout b int, variadic c int[]) language sql as 'select 1, 2'" - ), - [ - Some("a".to_string()), - Some("b".to_string()), - Some("c".to_string()) - ] - ); - } - - #[test] - fn array_type() { - let input = "CREATE FUNCTION add(test0 integer[], test1 integer) RETURNS integer - AS 'select $1 + $2;' - LANGUAGE SQL - IMMUTABLE - RETURNS NULL ON NULL INPUT;"; - - let ast = pgls_query::parse(input).unwrap().into_root().unwrap(); - - let sig = get_sql_fn_signature(&ast); - - assert!(sig.is_some()); - - let sig = sig.unwrap(); - - assert!( - sig.args - .iter() - .find(|arg| arg.type_.is_array) - .map(|arg| { - assert_eq!(arg.type_.name, "int4"); - assert!(arg.type_.is_array); - }) - .is_some() - ); - } - #[test] fn set_sql_function_body() { let input = "CREATE FUNCTION add(a integer, b integer) RETURNS integer