Do not warn on unused bindings whose names begin with _

[ikappaki]

Hi,

could you please review patch to not work on unused bindings whose name begins with _. It addresses #756.

I've addressed some of such warnings coming up when running the nrepl-server, and added a test.

Thanks

[chrisrink10]

The compiler already ignores symbols entitled _ for just this purpose.

I am ok with the patch for rare cases where _ may be insufficient, but in my opinion every one of the _ prefixed names you've included here should just be _ rather than _{name} as they are unused and their names are irrelevant.

[ikappaki]

The compiler already ignores symbols entitled _ for just this purpose.

I am ok with the patch for rare cases where _ may be insufficient, but in my opinion every one of the _ prefixed names you've included here should just be _ rather than _{name} as they are unused and their names are irrelevant.

Thank you, knowing the name of the thing that you want to ignore is very useful, I use it all the time. It provides more information about the current context.

[chrisrink10]

Upon further reflection, I don't think I agree with this patch. The compiler explicitly supports ^:no-warn-when-unused metadata for suppressing the warning in cases where a name is desired even if unused.

Thank you, knowing the name of the thing that you want to ignore is very useful, I use it all the time. It provides more information about the current context.

As to this point, there are cases where that may be true and others where I strongly disagree. For cases such as this example below, there is no value in having a name bound and it is often ignored in Python (by not binding a name) and in Clojure:

(catch python/Exception _e
   )

[ikappaki]

Upon further reflection, I don't think I agree with this patch. The compiler explicitly supports ^:no-warn-when-unused metadata for suppressing the warning in cases where a name is desired even if unused.

allowing a _ prefix gives the finest granularity down to the binding level, while a the above metadata value applies to everything in the scope of the metadata, so it's not the same.

Thank you, knowing the name of the thing that you want to ignore is very useful, I use it all the time. It provides more information about the current context.

As to this point, there are cases where that may be true and others where I strongly disagree. For cases such as this example below, there is no value in having a name bound and it is often ignored in Python (by not binding a name) and in Clojure:

(catch python/Exception _e
   )

I still find it useful that what we ignore even in this case has a name, name e which is what we usually use for the exception. Perhaps that is a personal preference.

btw, this is the standard behavior of the clj-kondo linter, here is an example from Calva in visual studio which uses clj-kondo for linting (the wiggly line inidcates an error, in this case unused binding):

[chrisrink10]

allowing a _ prefix gives the finest granularity down to the binding level, while a the above metadata value applies to everything in the scope of the metadata, so it's not the same.

The metadata ^:no-warn-when-unused is per-binding as you can see here:

basilisp/src/basilisp/core.lpy

Line 1768 in cb16d60

([^:no-warn-when-unused type aseq]

That being said, since you have indicated that this is something you find useful and is part of other community projects I am willing to accept the patch to suppress the Basilisp compiler's warnings with _ prefixed names.

However, I do not prefer that style and do not want to use it for code in this repository. In general I do not prefer to give bindings names where it is not required or where it is not serving an explicit documentation purpose. Therefore, I would ask that you please revert the suppressions included in this PR using this style to use _.

[ikappaki]

allowing a _ prefix gives the finest granularity down to the binding level, while a the above metadata value applies to everything in the scope of the metadata, so it's not the same.

The metadata ^:no-warn-when-unused is per-binding as you can see here:

basilisp/src/basilisp/core.lpy

Line 1768 in cb16d60

([^:no-warn-when-unused type aseq]

That being said, since you have indicated that this is something you find useful and is part of other community projects I am willing to accept the patch to suppress the Basilisp compiler's warnings with _ prefixed names.

However, I do not prefer that style and do not want to use it for code in this repository. In general I do not prefer to give bindings names where it is not required or where it is not serving an explicit documentation purpose. Therefore, I would ask that you please revert the suppressions included in this PR using this style to use _.

Great thanks, Updated unused bindings to '_', I didn't realise you can have metadata at the binding level, thanks for the tip.