Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error count does not match the number of errors outputted #2048

Closed
1 task done
aramikuto opened this issue Mar 11, 2024 · 5 comments · Fixed by #2277
Closed
1 task done

error count does not match the number of errors outputted #2048

aramikuto opened this issue Mar 11, 2024 · 5 comments · Fixed by #2277
Assignees
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@aramikuto
Copy link

Environment information

CLI:
  Version:                      1.6.0
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v18.19.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "pnpm/8.14.3"

Biome Configuration:
  Status:                       unset

Workspace:
  Open Documents:               0

Rule name

useImportType

Playground link

N/A

Expected result

The number of errors found in the output of both the check and lint commands should match the number of errors that were printed.

Let's create a file named a.ts with the following content:

import React from "react";
const a: React.ReactNode = null;

When executing the biome lint a.ts command, the output is:

a.ts:1:1 lint/style/useImportType  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ All these imports are only used as types.
  
  > 1 │ import React from "react";
      │ ^^^^^^^^^^^^^^^^^^^^^^^^^^
    2 │ const a: React.ReactNode = null;
    3 │ 
  
  ℹ Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
  
  ℹ Safe fix: Use import type.
  
    1 │ import·type·React·from·"react";
      │        +++++                   

Checked 1 file in 15ms. No fixes needed.
Found 2 errors.
lint ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.

It says Found 2 errors, but only one error is present.


When executing the biome check a.ts command, the output is:

a.ts:1:1 lint/style/useImportType  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ All these imports are only used as types.
  
  > 1 │ import React from "react";
      │ ^^^^^^^^^^^^^^^^^^^^^^^^^^
    2 │ const a: React.ReactNode = null;
    3 │ 
  
  ℹ Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
  
  ℹ Safe fix: Use import type.
  
    1 │ import·type·React·from·"react";
      │        +++++                   

Checked 1 file in 10ms. No fixes needed.
Found 3 errors.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ Some errors were emitted while running checks.

The printed errors are the same as in the lint case, but the error count is 3 for some reason now.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Conaclos Conaclos changed the title 💅 useImportType error count does not match the number of errors outputted error count does not match the number of errors outputted Mar 11, 2024
@Conaclos Conaclos added the A-Project Area: project label Mar 11, 2024
@ematipico ematipico added S-Help-wanted Status: you're familiar with the code base and want to help the project A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug and removed A-Project Area: project labels Mar 11, 2024
@chansuke
Copy link
Member

I'd like to work on this issue:)

@Sec-ant
Copy link
Member

Sec-ant commented Mar 17, 2024

Hello @chansuke, I did some debug for this issue just out of curiosity and for getting familiar with the codebase. The extra count of the error seems from this piece:

Message::Failure => {
self.errors.fetch_add(1, Ordering::Relaxed);
}

The way I debugged it is printing a message before each line of self.errors.fetch_add and ran the lint and check commands respectively. Here are the results.

  • cargo run --bin biome lint ./a.ts --verbose --diagnostic-level=info

    It says it found 2 errors, 1 of them is from the Message::Failure branch.

    image

  • cargo run --bin biome check ./a.ts --verbose --diagnostic-level=info

    It says it found 3 errors, 2 of them are from the Message::Failure branch.

    image

I think the reason is that the error count will be correctly increased in other dedicated branches, so the one in branch Message::Failure seems redundant. I not sure though if we should remove this branch. But by searching Message::Failure in the whole codebase, it seems that this message will only be sent from the lint and check processes.

I hope my findings can provide some help :)

@chansuke
Copy link
Member

@Sec-ant Hi!Thank you so much for the information!I was busy last week but start working on this issue tomorrow.

@Sec-ant
Copy link
Member

Sec-ant commented Apr 3, 2024

Hi @chansuke, I'm gonna take over this issue to ship a fix before v1.6.4 is released.

@Sec-ant Sec-ant assigned Sec-ant and unassigned chansuke Apr 3, 2024
@Sec-ant Sec-ant linked a pull request Apr 3, 2024 that will close this issue
@chansuke
Copy link
Member

chansuke commented Apr 3, 2024

@Sec-ant
NP. Thank you for your great work!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI S-Bug-confirmed Status: report has been confirmed as a valid bug S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants