From 5169d8bcb1f641cce2f33c1e12069e6e541571e5 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Fri, 19 Apr 2024 15:48:13 -0400 Subject: [PATCH] Support more granular unused method checks --- src/code_info/functionlike_info.rs | 5 +- src/code_info/issue.rs | 1 + src/code_info/method_info.rs | 24 ------- src/code_info_builder/functionlike_scanner.rs | 4 +- src/file_scanner_analyzer/populator.rs | 13 ---- src/file_scanner_analyzer/unused_symbols.rs | 64 +++++++++++++------ src/language_server/lib.rs | 7 ++ src/str/build.rs | 1 + tests/diff/xhpMissingAttribute/output.txt | 4 +- .../UnusedClass}/removeUnusedClass/input.hack | 0 .../UnusedClass}/removeUnusedClass/output.txt | 0 .../removeFunctionWithComments/input.hack | 0 .../removeFunctionWithComments/output.txt | 0 .../fix/UnusedPrivateMethod/simple/input.hack | 9 +++ .../fix/UnusedPrivateMethod/simple/output.txt | 7 ++ .../replacements.txt | 1 - .../removeUnusedClass/replacements.txt | 1 - 17 files changed, 80 insertions(+), 61 deletions(-) rename tests/{migrations/unused_symbol => fix/UnusedClass}/removeUnusedClass/input.hack (100%) rename tests/{migrations/unused_symbol => fix/UnusedClass}/removeUnusedClass/output.txt (100%) rename tests/{migrations/unused_symbol => fix/UnusedFunction}/removeFunctionWithComments/input.hack (100%) rename tests/{migrations/unused_symbol => fix/UnusedFunction}/removeFunctionWithComments/output.txt (100%) create mode 100644 tests/fix/UnusedPrivateMethod/simple/input.hack create mode 100644 tests/fix/UnusedPrivateMethod/simple/output.txt delete mode 100644 tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt delete mode 100644 tests/migrations/unused_symbol/removeUnusedClass/replacements.txt diff --git a/src/code_info/functionlike_info.rs b/src/code_info/functionlike_info.rs index 1135ac46..b27dd3e9 100644 --- a/src/code_info/functionlike_info.rs +++ b/src/code_info/functionlike_info.rs @@ -47,7 +47,7 @@ impl FnEffect { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] pub struct MetaStart { pub start_offset: u32, pub start_line: u32, @@ -150,6 +150,8 @@ pub struct FunctionLikeInfo { pub is_production_code: bool, pub is_closure: bool, + + pub overriding: bool, } impl FunctionLikeInfo { @@ -188,6 +190,7 @@ impl FunctionLikeInfo { is_production_code: true, pure_can_throw: false, is_closure: false, + overriding: false, } } diff --git a/src/code_info/issue.rs b/src/code_info/issue.rs index 8dca9c3b..5d1200a9 100644 --- a/src/code_info/issue.rs +++ b/src/code_info/issue.rs @@ -127,6 +127,7 @@ pub enum IssueKind { UnusedClosureParameter, UnusedFunction, UnusedFunctionCall, + UnusedInheritedMethod, UnusedInterface, UnusedParameter, UnusedPipeVariable, diff --git a/src/code_info/method_info.rs b/src/code_info/method_info.rs index 9d69f3f2..953a89ea 100644 --- a/src/code_info/method_info.rs +++ b/src/code_info/method_info.rs @@ -1,6 +1,3 @@ -use hakana_str::StrId; -use rustc_hash::FxHashSet; - use serde::{Deserialize, Serialize}; use crate::member_visibility::MemberVisibility; @@ -14,20 +11,6 @@ pub struct MethodInfo { pub is_final: bool, pub is_abstract: bool, - - pub defining_fqcln: Option, - - pub external_mutation_free: bool, - - pub immutable: bool, - - pub mutation_free_inferred: bool, - - pub this_property_mutations: Option>, - - pub stubbed: bool, - - pub probably_fluent: bool, } impl Default for MethodInfo { @@ -43,13 +26,6 @@ impl MethodInfo { visibility: MemberVisibility::Public, is_final: false, is_abstract: false, - defining_fqcln: None, - external_mutation_free: false, - immutable: false, - mutation_free_inferred: false, - this_property_mutations: None, - stubbed: false, - probably_fluent: false, } } } diff --git a/src/code_info_builder/functionlike_scanner.rs b/src/code_info_builder/functionlike_scanner.rs index 19d7e046..95acf124 100644 --- a/src/code_info_builder/functionlike_scanner.rs +++ b/src/code_info_builder/functionlike_scanner.rs @@ -90,7 +90,6 @@ pub(crate) fn scan_method( let mut method_info = MethodInfo::new(); - method_info.defining_fqcln = Some(classlike_name); method_info.is_static = m.static_; method_info.is_final = m.final_ || classlike_storage.is_final; method_info.is_abstract = m.abstract_; @@ -404,6 +403,9 @@ pub(crate) fn get_functionlike( StrId::CODEGEN => { functionlike_info.generated = true; } + StrId::OVERRIDE => { + functionlike_info.overriding = true; + } _ => {} } } diff --git a/src/file_scanner_analyzer/populator.rs b/src/file_scanner_analyzer/populator.rs index 287194c1..cbf6aa5c 100644 --- a/src/file_scanner_analyzer/populator.rs +++ b/src/file_scanner_analyzer/populator.rs @@ -436,19 +436,6 @@ fn populate_classlike_storage( // todo add file references for cache invalidation if storage.immutable { - for method_name in &storage.methods { - let functionlike_storage = codebase - .functionlike_infos - .get_mut(&(storage.name, *method_name)) - .unwrap(); - - if let Some(method_storage) = functionlike_storage.method_info.as_mut() { - if !method_storage.is_static { - method_storage.immutable = true; - } - } - } - for (_, property_storage) in storage.properties.iter_mut() { if !property_storage.is_static { property_storage.soft_readonly = true; diff --git a/src/file_scanner_analyzer/unused_symbols.rs b/src/file_scanner_analyzer/unused_symbols.rs index 0d93d862..11160f9c 100644 --- a/src/file_scanner_analyzer/unused_symbols.rs +++ b/src/file_scanner_analyzer/unused_symbols.rs @@ -97,10 +97,14 @@ pub(crate) fn find_unused_definitions( continue; } - if config - .migration_symbols - .contains_key(interner.lookup(&functionlike_name.0)) - { + let issue = Issue::new( + IssueKind::UnusedFunction, + format!("Unused function {}", interner.lookup(&functionlike_name.0)), + *pos, + &Some(FunctionLikeIdentifier::Function(functionlike_name.0)), + ); + + if config.issues_to_fix.contains(&issue.kind) && !config.add_fixmes { let meta_start = &functionlike_info.meta_start; let def_pos = &functionlike_info.def_location; analysis_result @@ -115,13 +119,6 @@ pub(crate) fn find_unused_definitions( ); } - let issue = Issue::new( - IssueKind::UnusedFunction, - format!("Unused function {}", interner.lookup(&functionlike_name.0)), - *pos, - &Some(FunctionLikeIdentifier::Function(functionlike_name.0)), - ); - if config.can_add_issue(&issue) { *analysis_result .issue_counts @@ -198,10 +195,7 @@ pub(crate) fn find_unused_definitions( &Some(FunctionLikeIdentifier::Function(*classlike_name)), ); - if config - .migration_symbols - .contains_key(interner.lookup(classlike_name)) - { + if config.issues_to_fix.contains(&issue.kind) && !config.add_fixmes { let meta_start = &classlike_info.meta_start; let def_pos = &classlike_info.def_location; analysis_result @@ -314,7 +308,13 @@ pub(crate) fn find_unused_definitions( } let issue = - if matches!(method_storage.visibility, MemberVisibility::Private) { + if matches!(method_storage.visibility, MemberVisibility::Private) + || (matches!( + method_storage.visibility, + MemberVisibility::Protected + ) && method_storage.is_final + && !functionlike_storage.overriding) + { Issue::new( IssueKind::UnusedPrivateMethod, format!( @@ -328,11 +328,25 @@ pub(crate) fn find_unused_definitions( *method_name_ptr, )), ) + } else if functionlike_storage.overriding { + Issue::new( + IssueKind::UnusedInheritedMethod, + format!( + "Unused inherited method {}::{}", + interner.lookup(classlike_name), + interner.lookup(method_name_ptr) + ), + functionlike_storage.name_location.unwrap(), + &Some(FunctionLikeIdentifier::Method( + *classlike_name, + *method_name_ptr, + )), + ) } else { Issue::new( IssueKind::UnusedPublicOrProtectedMethod, format!( - "Possibly-unused method {}::{}", + "Unused public or protected method {}::{}", interner.lookup(classlike_name), interner.lookup(method_name_ptr) ), @@ -350,7 +364,21 @@ pub(crate) fn find_unused_definitions( continue; } - if config.can_add_issue(&issue) { + if config.issues_to_fix.contains(&issue.kind) && !config.add_fixmes { + let meta_start = functionlike_storage.meta_start; + let def_pos = functionlike_storage.def_location; + analysis_result + .replacements + .entry(pos.file_path) + .or_default() + .insert( + (meta_start.start_offset, def_pos.end_offset), + Replacement::TrimPrecedingWhitespace( + meta_start.start_offset + 1 + - meta_start.start_column as u32, + ), + ); + } else if config.can_add_issue(&issue) { *analysis_result .issue_counts .entry(issue.kind.clone()) diff --git a/src/language_server/lib.rs b/src/language_server/lib.rs index bafc9c54..2c471d41 100644 --- a/src/language_server/lib.rs +++ b/src/language_server/lib.rs @@ -140,6 +140,13 @@ impl LanguageServer for Backend { } } + self.client + .log_message( + MessageType::INFO, + format!("receiving changes {:?}", new_file_statuses), + ) + .await; + if let Some(ref mut existing_file_changes) = self.file_changes { existing_file_changes.extend(new_file_statuses); } else { diff --git a/src/str/build.rs b/src/str/build.rs index 688350e0..f0dd1e72 100644 --- a/src/str/build.rs +++ b/src/str/build.rs @@ -258,6 +258,7 @@ fn main() -> Result<()> { "__EntryPoint", "__FILE__", "__FUNCTION__", + "__Override", "__PHP_Incomplete_Class", "__Sealed", "__construct", diff --git a/tests/diff/xhpMissingAttribute/output.txt b/tests/diff/xhpMissingAttribute/output.txt index a52755f4..5e80bb38 100644 --- a/tests/diff/xhpMissingAttribute/output.txt +++ b/tests/diff/xhpMissingAttribute/output.txt @@ -1,3 +1,3 @@ -ERROR: UnusedPublicOrProtectedMethod - input.hack:6:33 - Possibly-unused method MyElement::renderAsync +ERROR: UnusedPublicOrProtectedMethod - input.hack:6:33 - Unused public or protected method MyElement::renderAsync ERROR: UnusedFunction - input.hack:11:10 - Unused function foo -ERROR: MissingRequiredXhpAttribute - input.hack:12:12 - XHP class MyElement is missing a required attribute: some-attr \ No newline at end of file +ERROR: MissingRequiredXhpAttribute - input.hack:12:12 - XHP class MyElement is missing a required attribute: some-attr diff --git a/tests/migrations/unused_symbol/removeUnusedClass/input.hack b/tests/fix/UnusedClass/removeUnusedClass/input.hack similarity index 100% rename from tests/migrations/unused_symbol/removeUnusedClass/input.hack rename to tests/fix/UnusedClass/removeUnusedClass/input.hack diff --git a/tests/migrations/unused_symbol/removeUnusedClass/output.txt b/tests/fix/UnusedClass/removeUnusedClass/output.txt similarity index 100% rename from tests/migrations/unused_symbol/removeUnusedClass/output.txt rename to tests/fix/UnusedClass/removeUnusedClass/output.txt diff --git a/tests/migrations/unused_symbol/removeFunctionWithComments/input.hack b/tests/fix/UnusedFunction/removeFunctionWithComments/input.hack similarity index 100% rename from tests/migrations/unused_symbol/removeFunctionWithComments/input.hack rename to tests/fix/UnusedFunction/removeFunctionWithComments/input.hack diff --git a/tests/migrations/unused_symbol/removeFunctionWithComments/output.txt b/tests/fix/UnusedFunction/removeFunctionWithComments/output.txt similarity index 100% rename from tests/migrations/unused_symbol/removeFunctionWithComments/output.txt rename to tests/fix/UnusedFunction/removeFunctionWithComments/output.txt diff --git a/tests/fix/UnusedPrivateMethod/simple/input.hack b/tests/fix/UnusedPrivateMethod/simple/input.hack new file mode 100644 index 00000000..741da13d --- /dev/null +++ b/tests/fix/UnusedPrivateMethod/simple/input.hack @@ -0,0 +1,9 @@ +class Foo { + /** cool doc */ + private function bar(): void {} +} + +<<__EntryPoint>> +function main(): void { + new Foo(); +} \ No newline at end of file diff --git a/tests/fix/UnusedPrivateMethod/simple/output.txt b/tests/fix/UnusedPrivateMethod/simple/output.txt new file mode 100644 index 00000000..0db9166c --- /dev/null +++ b/tests/fix/UnusedPrivateMethod/simple/output.txt @@ -0,0 +1,7 @@ +class Foo { +} + +<<__EntryPoint>> +function main(): void { + new Foo(); +} \ No newline at end of file diff --git a/tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt b/tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt deleted file mode 100644 index 3f953866..00000000 --- a/tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt +++ /dev/null @@ -1 +0,0 @@ -baz \ No newline at end of file diff --git a/tests/migrations/unused_symbol/removeUnusedClass/replacements.txt b/tests/migrations/unused_symbol/removeUnusedClass/replacements.txt deleted file mode 100644 index 8c7e5a66..00000000 --- a/tests/migrations/unused_symbol/removeUnusedClass/replacements.txt +++ /dev/null @@ -1 +0,0 @@ -A \ No newline at end of file