withastro / language-tools

Language tools for Astro
MIT License
264 stars 52 forks source link

`astro check` reports that a used function is not used #476

Open jyasskin opened 1 year ago

jyasskin commented 1 year ago

What version of astro are you using?

1.9.2

Are you using an SSR adapter? If so, which one?

Node

What package manager are you using?

pnpm

What operating system are you using?

Linux

Describe the Bug

Running astro check on:

---
function noSuchRecipe(): Response {
  return new Response("<p>No such recipe</p>", {
    status: 404,
    statusText: "No such recipe",
    headers: {'content-type': 'text/html'},
  });
}

// Grab this recipe.
const { username, slug } = Astro.params;
if (!slug || !username) {
  return noSuchRecipe();
}
---

<p></p>

Returns:

/home/projects/github-urvrsi/src/pages/index.astro:2:10 Hint: 'noSuchRecipe' is declared but its value is never read.

but running the file actually shows the "No such recipe" output.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-urvrsi?file=src/pages/index.astro

Participation

Princesseuh commented 1 year ago

Thank you for reporting this issue! This effectively a bug.

This happens because of the shape of the generated TSX output, since the frontmatter is technically at the top level, the return is considered invalid and thus the function usage too, so it ends up reporting as unused.

Moving this to the language-tools repo since this is ultimately a language-server issue (which also affects the editor, though in different ways)

Mrowa96 commented 1 year ago

Hi, any plans to fix it in near future? Right now the only workaround is to suppress error by using @ts-expect-error which is far from perfect ;/

I'm willing to submit a PR but I would need some hints where to start.

Princesseuh commented 1 year ago

This is unfortunately a huge amount of work. We need to change the shape of the TSX output, which implies changing mappings, which tends to imply changing some manual mappings in completions / code actions as well.

If you're interested in tackling this, I can give further information, but it's a fair amount of work, in multiple programming languages and with also a fair amount of background needed 😅

Mrowa96 commented 1 year ago

Ok, so for now I will leave it, I will start with something minor 😅

Princesseuh commented 1 year ago

A non-technical alternative would be to create a proposal for this to be changed to a throw, instead of a return. You could create a proposal here for this: https://github.com/withastro/roadmap/discussions

Even it does look weird to throw things like that, it's actually a common pattern when wanting to top-level react to things and would be more valid JavaScript.

mb21 commented 1 year ago

Would this be easier to solve if we had script tags instead of frontmatter?

Princesseuh commented 1 year ago

Would this be easier to solve if we had script tags instead of frontmatter?

Nope, same problem. From the editor tooling perspective, it being a script tag or a frontmatter doesn't really change much

mb21 commented 1 year ago

I see. Apart from throwing or exposing some sort of main function to be called (which both seem awkward), I can see only one solution: Astro.response (or something like it) should be assignable and of type ResponseInit | Response so you could do:

if (!slug || !username) {
  Astro.response = noSuchRecipe();
  return
}
mb21 commented 1 year ago

since the frontmatter is technically at the top level

Or perhaps that's the root problem? Couldn't this be transparently fixed by astro by wrapping the frontmatter in a function (e.g. an Immediately Invoked Function Expression)?

Princesseuh commented 1 year ago

since the frontmatter is technically at the top level

Or perhaps that's the root problem? Couldn't this be transparently fixed by astro by wrapping the frontmatter in a function (e.g. an Immediately Invoked Function Expression)?

Yes and no, for two reasons:

It's honestly a problem that's way more complicated than it seems

mb21 commented 1 year ago

Okay, not sure I understood that 100% as I'm not familiar with the .astro compiler... (I was thinking of a hack to work on the string before it even is parsed into any AST... basically splitting the front-matter string after the imports, then wrapping the second part in a IIFE)

But anyway, if it's not doable, then I vote for Astro.response being assignable... perhaps using a proxy?

omidantilong commented 9 months ago

Just chiming in to say I've also encountered this, and it is very odd. In my case, I was able to suppress the TS error in the IDE (in VSCode at least) by re-assigning the import:

import someResponse from "../someFile"
const fancyResponse = someResponse
...
return fancyResponse()

It's more than a little clunky, but keeps the editor happy. However, astro check will still think fancyResponse is declared but never read.

I've not spent a huge amount of time with Astro, but so far I'm liking it - this is the first real ickiness I've encountered ❤️

OrlandoBricenoB commented 5 months ago

This is happening to me with astro check:

dist/_worker.js/chunks/pages/index_DliSBx1j.mjs:24:25 - warning ts(6133): 'receiver' is declared but its value is never read.

24       get(target, name, receiver) {
                           ~~~~~~~~

With other 13 hints more.

arty-name commented 5 months ago

@OrlandoBricenoB I believe in your case astro check goes through the built code in the dist folder, which is a separate issue

gersomvg commented 4 months ago

If you're open to a simple (hacky) heuristic that might be less work to implement, we could add a compiler step that takes any return x (that is not in a nested function scope) and turns it into const HASH123 = x; HASH123; return HASH123;.

Though that wouldn't solve the error from still showing up in my editor intermittently. Weird that it doesn't do that consistently 🤔

gersomvg commented 4 months ago

A non-technical alternative would be to create a proposal for this to be changed to a throw, instead of a return. You could create a proposal here for this: https://github.com/withastro/roadmap/discussions

I see there is already a proposal for this: https://github.com/withastro/roadmap/discussions/830

Eptagone commented 2 months ago

Same problem here. I had to remove astro check from build process because otherwise, it always fails.

lhansford commented 2 months ago

I'm using this workaround until there's a fix. It's a little bit of extra code but allows me to keep astro check in place.

I create a function that I can pass the function that astro check is flagging to. Doing so means that the checker now believes the function to be used.

export function avoidAstroTypeCheckBug(_: unknown) {
  // See https://github.com/withastro/language-tools/issues/476 for more context.
  return;
}

// In a .astro file
import { avoidAstroTypeCheckBug } from './avoidAstroTypeCheckBug';
import { createNotFoundResponse } from './createNotFoundResponse';

if (!someValue) {
  avoidAstroTypeCheckBug(createNotFoundResponse);
  return createNotFoundResponse();
}
millette commented 2 months ago

I had someting very similar to OP:

function noSuchRecipe(): Response {
  return new Response("<p>No such recipe</p>", {
    status: 404,
    statusText: "No such recipe",
    headers: {'content-type': 'text/html'},
  });
}

// Grab this recipe.
const { username, slug } = Astro.params;
if (!slug || !username) {
  return noSuchRecipe();
}

My workaround for now was to rename noSuchRecipe to _noSuchRecipe to silence the warning.

gersomvg commented 2 months ago

I'm using this workaround until there's a fix. It's a little bit of extra code but allows me to keep astro check in place.

I create a function that I can pass the function that astro check is flagging to. Doing so means that the checker now believes the function to be used.

export function avoidAstroTypeCheckBug(_: unknown) {
  // See https://github.com/withastro/language-tools/issues/476 for more context.
  return;
}

// In a .astro file
import { avoidAstroTypeCheckBug } from './avoidAstroTypeCheckBug';
import { createNotFoundResponse } from './createNotFoundResponse';

if (!someValue) {
  avoidAstroTypeCheckBug(createNotFoundResponse);
  return createNotFoundResponse();
}

You could just do:

createNotFoundResponse;
return createNotFoundResponse();

I guess yours is more explanatory

Princesseuh commented 2 months ago

I'm working on a fix here https://github.com/withastro/compiler/pull/1028

gersomvg commented 2 months ago

@Princesseuh Amazing! I guess that means throw syntax will also be explicitly supported? I've been using that since a few weeks with my own middleware and I haven't run into any difficulties. Would be nice to have it natively supported ❤️

Princesseuh commented 2 months ago

@Princesseuh Amazing! I guess that means throw syntax will also be explicitly supported? I've been using that since a few weeks with my own middleware and I haven't run into any difficulties. Would be nice to have it natively supported ❤️

No, I'm only working on making it so top-level returns don't trigger an error in editor tooling + astro check, no changes to Astro itself.

lhansford commented 2 months ago

:tada: thanks @Princesseuh!

Princesseuh commented 2 months ago

Re-opening this as I'm about to revert the fix. It did work for most cases, but some cases broke it horribly and led to confusing errors. Currently trying to work on a fix, but it is quite tricky.

Apologies for the inconvenience!

millette commented 2 months ago

Thanks for the hard work @Princesseuh

Going back to my workaround https://github.com/withastro/language-tools/issues/476#issuecomment-2235221254

mlafeldt commented 1 week ago

My workaround for now was to rename noSuchRecipe to _noSuchRecipe to silence the warning.

Good idea! Works for me too.

-const { userId, redirectToSignIn } = Astro.locals.auth()
+const { userId, redirectToSignIn: _redirectToSignIn } = Astro.locals.auth()
 if (!userId) {
-  return redirectToSignIn()
+  return _redirectToSignIn()
 }