Support more granular unused method checks

slackhq · Apr 19, 2024 · 5169d8b · 5169d8b
1 parent 3d81aa9
commit 5169d8b
Show file tree

Hide file tree

Showing 17 changed files with 80 additions and 61 deletions.
diff --git 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
@@ -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
@@ -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<StrId>,
-
-    pub external_mutation_free: bool,
-
-    pub immutable: bool,
-
-    pub mutation_free_inferred: bool,
-
-    pub this_property_mutations: Option<FxHashSet<String>>,
-
-    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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+ERROR: MissingRequiredXhpAttribute - input.hack:12:12 - XHP class MyElement is missing a required attribute: some-attr
diff --git a/...nused_symbol/removeUnusedClass/input.hack → .../UnusedClass/removeUnusedClass/input.hack b/...nused_symbol/removeUnusedClass/input.hack → .../UnusedClass/removeUnusedClass/input.hack
diff --git a/...nused_symbol/removeUnusedClass/output.txt → .../UnusedClass/removeUnusedClass/output.txt b/...nused_symbol/removeUnusedClass/output.txt → .../UnusedClass/removeUnusedClass/output.txt
diff --git a/...bol/removeFunctionWithComments/input.hack → ...ion/removeFunctionWithComments/input.hack b/...bol/removeFunctionWithComments/input.hack → ...ion/removeFunctionWithComments/input.hack
diff --git a/...bol/removeFunctionWithComments/output.txt → ...ion/removeFunctionWithComments/output.txt b/...bol/removeFunctionWithComments/output.txt → ...ion/removeFunctionWithComments/output.txt
diff --git a/tests/fix/UnusedPrivateMethod/simple/input.hack 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();
+}
diff --git a/tests/fix/UnusedPrivateMethod/simple/output.txt b/tests/fix/UnusedPrivateMethod/simple/output.txt
@@ -0,0 +1,7 @@
+class Foo {
+}
+
+<<__EntryPoint>>
+function main(): void {
+    new Foo();
+}
diff --git a/tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt b/tests/migrations/unused_symbol/removeFunctionWithComments/replacements.txt
diff --git a/tests/migrations/unused_symbol/removeUnusedClass/replacements.txt b/tests/migrations/unused_symbol/removeUnusedClass/replacements.txt