Fix/apply lint

[niyarin]

I fixed almost bad coding style, except Boxed-math, which I couldn't fix.

Clj-kondo

master

> find src/ |grep clj|xargs clj-kondo --lint
linting took 1340ms, errors: 1, warnings: 85
> find test/ |grep clj|xargs clj-kondo --lint
linting took 2115ms, errors: 1, warnings: 3

This PR

> find src/ |grep clj|xargs clj-kondo --lint
linting took 1325ms, errors: 0, warnings: 0
> find test/ |grep clj|xargs clj-kondo --lint
test/cljam/io/gff_test.clj:319:70: error: Unresolved symbol: ?str
linting took 2240ms, errors: 1, warnings: 0

Eastwood (exclude boxed-math)

Master

> lein eastwood "{:linters [:all] :exclude-linters [:keyword-typos :unused-namespaces :boxed-math] }"
== Warnings: 11. Exceptions thrown: 0

This PR

> lein eastwood "{:linters [:all] :exclude-linters [:keyword-typos :unused-namespaces :boxed-math] }"
== Warnings: 0. Exceptions thrown: 0

[codecov]

Codecov Report

Merging #269 (cbff4fc) into master (38b7918) will decrease coverage by 0.01%.
The diff coverage is 88.15%.

❗ Current head cbff4fc differs from pull request most recent head 004266f. Consider uploading reports for the commit 004266f to get more accurate results

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
- Coverage   88.85%   88.85%   -0.01%     
==========================================
  Files          78       78              
  Lines        6444     6496      +52     
  Branches      454      456       +2     
==========================================
+ Hits         5726     5772      +46     
- Misses        264      268       +4     
- Partials      454      456       +2     

Impacted Files	Coverage Δ
src/cljam/algo/convert.clj	90.00% <0.00%> (ø)
src/cljam/io/sam/reader.clj	78.08% <0.00%> (ø)
src/cljam/io/sam/util/flag.clj	100.00% <ø> (ø)
src/cljam/io/util/bin.clj	98.21% <ø> (ø)
src/cljam/io/util/chunk.clj	75.60% <ø> (ø)
src/cljam/tools/cli.clj	59.74% <ø> (ø)
src/cljam/util/sequence.clj	78.94% <33.33%> (ø)
src/cljam/io/vcf/reader.clj	87.34% <55.55%> (-0.39%)	⬇️
src/cljam/io/pileup.clj	86.36% <58.82%> (-2.70%)	⬇️
src/cljam/io/sam/util/cigar.clj	94.68% <66.66%> (+0.05%)	⬆️
... and 44 more

[r6eve]

Thanks for the PR. I checked that changes doesn't break the orignal codes, and it sounds good to me.

[alumi]

Thank you for working on this, @niyarin! I've added some comments.

I would like to take this chance to resolve boxed maths, too.
After playing around with the codes, I found that there are several varieties of this problem.
The biggest difference is that the warning is raised by clojure compiler or by eastwood. It seems like that there are cases that the compiler handles primitive types properly but eastwood can't recognize the types.
For example, cljam.io.util.bgzf/get-block-address is reported as boxed math by lein eastwood '{:linters [:boxed-math] :namespaces [cljam.io.util.bgzf]}' but not by lein update-in :global-vars assoc '*unchecked-math*' :warn-on-boxed -- check.
This is because of ^:const metadata attached to the vars. The compiler perform inlining of such vars to replace them with its values which are primitive numeric literals, obviously typed.

(set! *unchecked-math* :warn-on-boxed)
(def x 1)
(+ x 1)
;; Boxed math warning, call: public static java.lang.Number clojure.lang.Numbers.unchecked_add(java.lang.Object,long).
(def ^:const x 1)
(+ x 1)

So this is a bug in the linter and there is no practical problem at all, and they will be compiled and executed as a primitive-type arithmetic operation. This kind of reports can be ignored.

For cases that affect the actual bytecode, let's use this PR to correct the problem, checking with lein check and not eastwood.

First, we will address function definitions as follows.
For functions that always return a non-null primitive value, prefix the return primitive hint with the parameter vector. (e.g. cljam.common/get-exec-n-threads) This will make the return type primitive on direct caller via var.

Also, attach a primitive hint to a parameter that is always a primitive value. Note that the parameter symbol need to be top-level and must not be destructured. (e.g. cljam.io.bam-index.reader/read-bin-index*!) This makes the type of parameters in function definitions primitive, and also allows function calls directly via var to be made with primitive arguments.

A reference to a dynamic var that is bound to a primitive value is not resolved to a primitive type as is, so we need to cast it. (e.g. cljam.common/*n-threads*) Since other than long/double can be used for casts, select the appropriate size.
A var bound to a primitive value that can be ^:const should be marked as ^:const. (e.g. cljam.io.bam.encoder/fixed-tag-size)
Non-primitive values that are ^:const do not need a type hint. (e.g. cljam.io.csi/csi-magic)

For destructuring of parameters, local bindings, and partial expressions, avoid primitive hints and cast instead. (e.g. cljam.io.bcf.reader/reader)

However, the cast from Character to byte/short fails when *unchecked-math* is not nil, so cast it toint before casting to byte/short. (e.g. cljam.io.pileup/upper-table)

(set! *unchecked-math* nil)
(byte \A) ;; => 65
(short \A) ;; => 65
(int \A) ;; => 65
(long \A) ;; => 65
(set! *unchecked-math* :warn-on-boxed)
(byte \A)
;; Execution error (ClassCastException)
;; class java.lang.Character cannot be cast to class java.lang.Number (java.lang.Character and java.lang.Number are in module java.base of loader 'bootstrap')
(short \A)
;; Execution error (ClassCastException)
;; class java.lang.Character cannot be cast to class java.lang.Number (java.lang.Character and java.lang.Number are in module java.base of loader 'bootstrap')
(int \A) ;; => 65
(long \A) ;; => 65

I have pushed an incomplete commit to spike/boxed-math and hope it will be helpful.

[alumi]

@niyarin Thank you for updating the PR! I added a few extra comments, but basically the changes seem to be fine👍.

[niyarin]

@alumi
Thank you for pointing.
I fixed them.

[niyarin]

I'll add :reflectoin to eastwood exclude linters to avoid this warnings.

Warning: [eastwood][reflection] call to method charAt can't be resolved.
Warning: [eastwood][reflection] call to method put on java.nio.CharBuffer can't be resolved (argument types: unknown).
Warning: [eastwood][reflection] call to method charAt can't be resolved.
Warning: [eastwood][reflection] call to method put on java.nio.CharBuffer can't be resolved (argument types: unknown).

[alumi]

Thank you for updating. There seems to be several points left that need to be fixed.

• Fix/apply lint #269 (comment)
• Fix/apply lint #269 (comment)
• Fix/apply lint #269 (comment)

[niyarin]

@alumi
I fixed what was not fixed.

[alumi]

Thank you @niyarin! LGTM 👍
Would you squash your commits as follows before merging?

1. Replace ::set-output with $GITHUB_OUTPUT
2. Change linter options
3. Fix return type hints
4. Fix boxed math warnings

[alumi]

@niyarin Additionally, if you have time, would you consider whether some optional (default level is :off) linters of clj-kondo could be enabled?
For example, :missing-docstring, :single-key-in, :shadowed-var, and :unused-value seem worth enabling.
If there is a new warning, I would appreciate it if you could correct it in another PR.