denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
97.28k stars 5.36k forks source link

`deno fmt` panics with `https://esm.sh/v131/readable-stream@2.3.8/denonext/readable-stream.mjs` #24444

Open kt3k opened 4 months ago

kt3k commented 4 months ago

deno fmt panics at cli/tools/fmt.rs:475 when formatting https://esm.sh/v131/readable-stream@2.3.8/denonext/readable-stream.mjs

curl -o example.js https://esm.sh/v131/readable-stream@2.3.8/denonext/readable-stream.mjs
deno fmt example.js

Version: Deno 1.44.4

yazan-abdalrahman commented 3 months ago

@kt3k Hello, what is expected? Should the file be formatted or changed panic to error?

kt3k commented 3 months ago

The file should be formatted in my view as it doesn't seem having a syntax error.

yazan-abdalrahman commented 3 months ago

The file should be formatted in my view as it doesn't seem having a syntax error. @kt3k

I'll cheek that. But as I saw it attempted many times, then in code a specific number of tries, if it failed to format the file in this number of tries, it would throw panic, therefore this section i think it should be converted to error and also when catch errors t throw panic it should change it also

kt3k commented 3 months ago

I don't think it makes any difference if it's changed to error (The users can't do anything in either case).

I think this needs to be addressed in upstream formatter, dprint

yazan-abdalrahman commented 3 months ago

I don't think it makes any difference if it's changed to error (The users can't do anything in either case).

I think this needs to be addressed in upstream formatter, dprint

@kt3k
Okay, I checked if we have errors in syntax, no syntax error. I attempted separating the code into various pieces to find the code that was causing the problem, but when I created some portions and formatted them, the format worked in those parts. Perhaps the issue is due to length or some code that should be on a new line.

yazan-abdalrahman commented 3 months ago

@kt3k I increased the number of attempts to format, and it worked.

PS D:\yad\deno> cargo build Compiling deno v1.44.4 (D:\yad\deno\cli) Finished dev profile [unoptimized + debuginfo] target(s) in 1m 01s PS D:\yad\deno> curl -o example.js https://esm.sh/v131/readable-stream@2.3.8/denonext/readable-stream.mjs
PS D:\yad\deno> .\target\debug\deno fmt .\example.js
[cli\tools\fmt.rs:450:9] &file_path = "D:\yad\deno\example.js" [cli\tools\fmt.rs:451:9] &fmt_options = FmtOptionsConfig { use_tabs: None, line_width: None, indent_width: None, single_quote: None, prose_wrap: None, semi_colons: None, } [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:1" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:2" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:3" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:4" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:5" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:6" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:7" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:8" [cli\tools\fmt.rs:556:9] format!("attempt:{}",count) = "attempt:9" D:\yad\deno\example.js Checked 1 file

image image

yazan-abdalrahman commented 3 months ago

@kt3k I might just increase the number of formatting attempts, or we might continue investigation to find better alternatives.

yazan-abdalrahman commented 3 months ago

@kt3k

As I understand it, it tries a few attempts and compares them to ensure stability of format, thus I need to investigate why it isn't stable and not raise the attempt numbers.

kt3k commented 3 months ago

thus I need to investigate why it isn't stable

That'd be very helpful!

yazan-abdalrahman commented 3 months ago

@kt3k I tried to investigate the issue on stability on the result of format_text in dprint-plugin-typescript, thus it needs more investment in dprint-plugin-typescript, then also dprint_core, which I tried but I found it hard to understand the issue.

As I noticed, each time we ran dprint_plugin_typescript::format_text on his result, there were some format changes, but after a few attempts, it be stable, and the behavior in fmt::format_ensure_stable it's to get the format for origin_txt by dprint_plugin_typescript::format_text then the result of format it be as current_text then we will format the current_text by dprint_plugin_typescript::format_text checke if result of format current_text match with current_text and it will attempt that until get first match between formatted text and result of f the formatted text or finish the number of attempts.

so we can increase the number of attempts that will resolve issue , or remove double format check 'check if result of formatted text if it formatted again it gives the same result ' but this I think it not preferred because the origin behavior know it not stable so it gives u number of attempt to verify ,and it check each time if the latest result of format match with next result of format until get match it will stop attempt so it but default number of attempt to prevent infinity Loob so I think it best choice to increase the number off attempt

check this code to understand

/// When storing any formatted text in the incremental cache, we want
/// to ensure that anything stored when formatted will have itself as
/// the output as well. This is to prevent "double format" issues where
/// a user formats their code locally and it fails on the CI afterwards.
fn format_ensure_stable(
  file_path: &Path,
  file_text: &str,
  fmt_options: &FmtOptionsConfig,
  fmt_func: impl Fn(
    &Path,
    &str,
    &FmtOptionsConfig,
  ) -> Result<Option<String>, AnyError>,
) -> Result<Option<String>, AnyError> {
  let formatted_text = fmt_func(file_path, file_text, fmt_options)?;

  match formatted_text {
    Some(mut current_text) => {
      let mut count = 0;
      loop {
        match fmt_func(file_path, &current_text, fmt_options) {
          Ok(Some(next_pass_text)) => {
            // just in case
            if next_pass_text == current_text {
              return Ok(Some(next_pass_text));
            }
            current_text = next_pass_text;
          }
          Ok(None) => {
            return Ok(Some(current_text));
          }
          Err(err) => {
            panic!(
              concat!(
                "Formatting succeeded initially, but failed when ensuring a ",
                "stable format. This indicates a bug in the formatter where ",
                "the text it produces is not syntactically correct. As a temporary ",
                "workaround you can ignore this file ({}).\n\n{:#}"
              ),
              file_path.display(),
              err,
            )
          }
        }
        count += 1;
        if count == 5 {
          panic!(
            concat!(
              "Formatting not stable. Bailed after {} tries. This indicates a bug ",
              "in the formatter where it formats the file ({}) differently each time. As a ",
              "temporary workaround you can ignore this file."
            ),
            count,
            file_path.display(),
          )
        }
      }
    }
    None => Ok(None),
  }
}