search

aristanetworks / purescript-backend-optimizer

Optimizing backend toolkit and modern ECMAScript backend for PureScript
MIT License
201 stars 18 forks source link

`unsafeThaw` inlining breaks Array.length checks with new SemRef handling. #92

Closed natefaubion closed 1 year ago

natefaubion commented 1 year ago

Noticed in codegen of Arrays@7.2.0.

There's a backend specific inlining rule for unsafeThaw which causes it to inline into a call to pure. https://github.com/aristanetworks/purescript-backend-optimizer/blob/25d8ae89a650a12acfbca2b05e3715bd2aa5f2b3/backend-es/src/PureScript/Backend/Optimizer/Codegen/EcmaScript/Foreign.purs#L95

This interacts poorly with the new SemRef tracking we added, because the optimizer now considers something like:

example = ST.run
  arr <- unsafeThaw [1, 2, 3]
  ...

To be a normal immutable array binding for the purposes of analysis. arr here is known to have a length of 3. This breaks Data.Array.nubBy, which uses the equivalent of Array.length <$> unsafeFreeze arr in a loop. https://github.com/purescript/purescript-arrays/blob/72861214e541f3e072041d193f32ce66af9a456b/src/Data/Array.purs#L1121

The optimizer will treat that call to Array.length as a known constant even though it's changing.

It's arguable that this is within the realm of unsafety. This function is using a lot of unsafeNess, after all. But I think since unsafeThaw is a foreign implementation, this is on us. If unsafeThaw had been implemented in pure PureScript as unsafeCoerce then I would consider this a bug in arrays.

This doesn't break nubBy on 7.2.1 due to other issues, specifically, last no longer inlines.