fix(check): ignore certain diagnostics in remote modules and when publishing (#23119)

Unused locals and parameters don't make sense to surface in remote
modules. Additionally, fast check can cause these kind of diagnostics
when publishing, so they should be ignored.

Closes #22959

8 files changed, 73 insertions, 25 deletions

diff --git a/cli/tools/check.rs b/cli/tools/check.rs
index ec4490017..87ec88a4a 100644
--- a/cli/tools/check.rs
+++ b/cli/tools/check.rs
@@ -200,28 +200,13 @@ impl TypeChecker {
       check_mode: type_check_mode,
     })?;
 
-    let mut diagnostics = if type_check_mode == TypeCheckMode::Local {
-      response.diagnostics.filter(|d| {
-        if let Some(file_name) = &d.file_name {
-          if !file_name.starts_with("http") {
-            if ModuleSpecifier::parse(file_name)
-              .map(|specifier| !self.node_resolver.in_npm_package(&specifier))
-              .unwrap_or(true)
-            {
-              Some(d.clone())
-            } else {
-              None
-            }
-          } else {
-            None
-          }
-        } else {
-          Some(d.clone())
-        }
-      })
-    } else {
-      response.diagnostics
-    };
+    let mut diagnostics = response.diagnostics.filter(|d| {
+      if self.is_remote_diagnostic(d) {
+        type_check_mode == TypeCheckMode::All && d.include_when_remote()
+      } else {
+        true
+      }
+    });
 
     diagnostics.apply_fast_check_source_maps(&graph);
 
@@ -239,6 +224,20 @@ impl TypeChecker {
 
     Ok((graph, diagnostics))
   }
+
+  fn is_remote_diagnostic(&self, d: &tsc::Diagnostic) -> bool {
+    let Some(file_name) = &d.file_name else {
+      return false;
+    };
+    if file_name.starts_with("https://") || file_name.starts_with("http://") {
+      return true;
+    }
+    // check if in an npm package
+    let Ok(specifier) = ModuleSpecifier::parse(file_name) else {
+      return false;
+    };
+    self.node_resolver.in_npm_package(&specifier)
+  }
 }
 
 enum CheckHashResult {
diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs
index b40aeaeb4..3d13a52b7 100644
--- a/cli/tools/registry/mod.rs
+++ b/cli/tools/registry/mod.rs
@@ -924,6 +924,10 @@ async fn build_and_check_graph_for_publish(
           },
         )
         .await?;
+      // ignore unused parameter diagnostics that may occur due to fast check
+      // not having function body implementations
+      let check_diagnostics =
+        check_diagnostics.filter(|d| d.include_when_remote());
       if !check_diagnostics.is_empty() {
         bail!(
           concat!(
diff --git a/cli/tsc/diagnostics.rs b/cli/tsc/diagnostics.rs
index 362385c07..9525d1855 100644
--- a/cli/tsc/diagnostics.rs
+++ b/cli/tsc/diagnostics.rs
@@ -136,6 +136,13 @@ pub struct Diagnostic {
 }
 
 impl Diagnostic {
+  /// If this diagnostic should be included when it comes from a remote module.
+  pub fn include_when_remote(&self) -> bool {
+    /// TS6133: value is declared but its value is never read (noUnusedParameters and noUnusedLocals)
+    const TS6133: u64 = 6133;
+    self.code != TS6133
+  }
+
   fn fmt_category_and_code(&self, f: &mut fmt::Formatter) -> fmt::Result {
     let category = match self.category {
       DiagnosticCategory::Error => "ERROR",
@@ -275,11 +282,11 @@ impl Diagnostics {
 
   /// Return a set of diagnostics where only the values where the predicate
   /// returns `true` are included.
-  pub fn filter<P>(&self, predicate: P) -> Self
+  pub fn filter<P>(self, predicate: P) -> Self
   where
-    P: FnMut(&Diagnostic) -> Option<Diagnostic>,
+    P: FnMut(&Diagnostic) -> bool,
   {
-    let diagnostics = self.0.iter().filter_map(predicate).collect();
+    let diagnostics = self.0.into_iter().filter(predicate).collect();
     Self(diagnostics)
   }
 
diff --git a/tests/specs/jsr/no_unused_params/__test__.jsonc b/tests/specs/jsr/no_unused_params/__test__.jsonc
new file mode 100644
index 000000000..4192ad5dd
--- /dev/null
+++ b/tests/specs/jsr/no_unused_params/__test__.jsonc
@@ -0,0 +1,10 @@
+{
+  "base": "jsr",
+  "steps": [{
+    "args": "check --all main.ts",
+    "output": "main.out"
+  }, {
+    "args": "publish --dry-run",
+    "output": "publish.out"
+  }]
+}
diff --git a/tests/specs/jsr/no_unused_params/deno.json b/tests/specs/jsr/no_unused_params/deno.json
new file mode 100644
index 000000000..41a02bf40
--- /dev/null
+++ b/tests/specs/jsr/no_unused_params/deno.json
@@ -0,0 +1,9 @@
+{
+  "name": "@scope/package",
+  "version": "0.0.0",
+  "exports": "./main.ts",
+  "lock": false,
+  "compilerOptions": {
+    "noUnusedParameters": true
+  }
+}
diff --git a/tests/specs/jsr/no_unused_params/main.out b/tests/specs/jsr/no_unused_params/main.out
new file mode 100644
index 000000000..7f586ac8b
--- /dev/null
+++ b/tests/specs/jsr/no_unused_params/main.out
@@ -0,0 +1,4 @@
+Download http://127.0.0.1:4250/@denotest/add/meta.json
+Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json
+Download http://127.0.0.1:4250/@denotest/add/1.0.0/mod.ts
+Check file:///[WILDLINE]/no_unused_params/main.ts
diff --git a/tests/specs/jsr/no_unused_params/main.ts b/tests/specs/jsr/no_unused_params/main.ts
new file mode 100644
index 000000000..e8b12ca89
--- /dev/null
+++ b/tests/specs/jsr/no_unused_params/main.ts
@@ -0,0 +1,5 @@
+import * as inner from "jsr:@denotest/add";
+
+export function add(a: number, b: number): number {
+  return inner.add(a, b);
+}
diff --git a/tests/specs/jsr/no_unused_params/publish.out b/tests/specs/jsr/no_unused_params/publish.out
new file mode 100644
index 000000000..a8eff6eb3
--- /dev/null
+++ b/tests/specs/jsr/no_unused_params/publish.out
@@ -0,0 +1,10 @@
+Check file:///[WILDLINE]/main.ts
+Checking for slow types in the public API...
+Check file:///[WILDLINE]/main.ts
+Simulating publish of @scope/package@0.0.0 with files:
+   file:///[WILDLINE]/__test__.jsonc ([WILDLINE])
+   file:///[WILDLINE]/deno.json ([WILDLINE])
+   file:///[WILDLINE]/main.out ([WILDLINE])
+   file:///[WILDLINE]/main.ts ([WILDLINE])
+   file:///[WILDLINE]/publish.out ([WILDLINE])
+Warning Aborting due to --dry-run