deno icon indicating copy to clipboard operation
deno copied to clipboard

deno(fmt): adds optional semicolon thinking it's not optional

Open BlackAsLight opened this issue 1 year ago • 4 comments

Version: Deno 1.44.4

When the semicolon option is set to false for deno fmt, Deno adds the mandatory semicolons at the start of a line instead of at the end of the previous line.

It for some reason falsely thinks this line ++x requires a semicolon at the start ;++x, but this is not the case as with or without the semicolon, the interpretation of the code does not change, and I can't seem to think of one instance where something like this would be valid code.

x
++

Reproduction

Before

let x = 0
++x

After

let x = 0
;++x

BlackAsLight avatar Jun 20 '24 04:06 BlackAsLight

@dsherret probably there is a rule you have that if a line starts with +, you insert a semicolon. However that is not necessary for ++ and --, because they are prefixed with a no-newline-terminator-here.

lucacasonato avatar Jun 20 '24 07:06 lucacasonato

@lucacasonato @BlackAsLight @dsherret can provide a proper example when we add a semicolon as a prefix with + or -.

I think we should remove + - from the is_prefix_semi_colon_insertion_char rule ? or just prevent ++ -- case

yazan-abdalrahman avatar Jul 10 '24 09:07 yazan-abdalrahman

I think we should remove + - from the is_prefix_semi_colon_insertion_char rule ? or just prevent ++ -- case

While a semicolon before a ++x won't change the way it's interpreted, a semicolon before +x can change the way it's interpreted, as prefixing + and - to a variable is a way to implicitly convert the value to a number.

With the below code, a semicolon prefixing the last line will change the way it's interpreted from the value of y being '55' or '555'.

let x = '5'
let y = x + x
+x

On the other hand though, a line that started with ;+x would be a pointless expression so one could argue that if a line starts with + then it is meant to be part of the previous line and no semicolon should be added.

BlackAsLight avatar Jul 10 '24 10:07 BlackAsLight

I think we should remove + - from the is_prefix_semi_colon_insertion_char rule ? or just prevent ++ -- case

While a semicolon before a ++x won't change the way it's interpreted, a semicolon before +x can change the way it's interpreted, as prefixing + and - to a variable is a way to implicitly convert the value to a number.

With the below code, a semicolon prefixing the last line will change the way it's interpreted from the value of y being '55' or '555'.

let x = '5'
let y = x + x
+x

On the other hand though, a line that started with ;+x would be a pointless expression so one could argue that if a line starts with + then it is meant to be part of the previous line and no semicolon should be added.

Yes, I understand and I did that. I've introduced a case for '+x' and '-x' that should add ; to be ';+x' ';-x', and I've prevented add ; in the ++ -- scenario.

check this PR it has test explaining all cases https://github.com/dprint/dprint-plugin-typescript/pull/648

`

== shouldn't insert semi-colons at the start of lines beginning with a bracket == let x = 0 ++x

[expect] let x = 0 ++x

== shouldn't insert semi-colons at the start of lines beginning with a bracket == let x = 0 --x

[expect] let x = 0 --x

== should insert semi-colons at the start of lines beginning with a bracket == +5

[expect] ;+5

== should insert semi-colons at the start of lines beginning with a bracket == -5

[expect] ;-5 `

yazan-abdalrahman avatar Jul 10 '24 11:07 yazan-abdalrahman

Hello, not sure if this is the same issue or not, but I also tried using no semicolons with fmt and Deno inserts a semicolon in a very unexpected place.

Minimal repro code:

let resp = [1]
let status = null

if (Array.isArray(resp)) {
  [status] = resp
}

After running deno fmt:

let resp = [1]
let status = null

if (Array.isArray(resp)) {
  ;[status] = resp
}

A semicolon was added before [status]

hourianto avatar Jul 17 '24 03:07 hourianto

@hourianto that one is expected. Prettier does the same thing: https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAbOMAEAnOBnAB0wF5MBtARgF0AdKdLPGAQxgFc8TMo3VU66ASwBmmABQBBbNmYBPAHSC8UmbLG5CASk2ZgdTOSasOVLhoJ0AviAA0ICARiDoeZKGbSIAdwAKHhK4ozKhecq52AEYyYADWGADKzAC2cAAyglBwyMLBeHCR0XEw8QTMYBkA5sgw2Gz5IHlJgtm59XAAHgRw2IIpsMEAKt1QHoL4Lah5dniV6ACKbBDwE1MgAFZ47fGzcAtLWUg5k-UAjovwPtgOgSDMeAC0mXAAJi+2IDXMgqiVAMIQSSSzGQtz47xmUAq6AkMBqggibAu3XSmRW9QAFjAkqgAOrowTwQhlODxAIEwQANwJshBYDw4RAFLqAEkoK9YPEwD1HBI2fEYLJ0Gi7AQrnkcTICCDRfhuhSsnYMnlsDBLswKkDhSBSthlSCIswInBUO9RRkYDjBM8YOjkAAOAAMdlwZ0EuDVGuBh1adhYEUt1ttSAATHYOHABobAkdVnAkkbnq9nqlmJC2Oq4AAxCDYIGwyog5iIiAgSyWIA

The reason is because if you add a statement before that line it could merge with that one causing a bug.

dsherret avatar Jul 17 '24 04:07 dsherret

@dsherret thanks for the info, I'm quite new with TS so it just looked very out of place :)

hourianto avatar Jul 17 '24 06:07 hourianto

@dsherret , @BlackAsLight

Hello, is my solution acceptable? Please take a look.

yazan-abdalrahman avatar Jul 17 '24 06:07 yazan-abdalrahman

Fixed in https://github.com/dprint/dprint-plugin-typescript/pull/648 via dprint-plugin-typescript upgrade in https://github.com/denoland/deno/pull/24819

dsherret avatar Aug 01 '24 15:08 dsherret