Refactor array bracket access to dedicated AST node

[shulhi]

Refactors array bracket access from Pexp_apply(Array.get/set, ...) to a dedicated Pexp_index AST node. This is an internal refactoring with no user-facing changes.

Changes:

• arr[0] now represented as Pexp_index(arr, 0, None) in AST
• arr[0] = val now represented as Pexp_index(arr, 0, Some(val)) in AST
• Same type checking, same JavaScript output, same behavior

Why:

• Cleaner AST representation
• Sets up infrastructure for future dict support (Dict : Accessing items with bracket, like an array or object #8163)

[nojaf]

Hmm, any reason you went with one new node to represent two different things?
If feel like the optional expression is not the cleanest way to model this.
Would it not make more sense to just split these?
Is this to remain consistent with something?

[pkg-pr-new]

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@8168

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@8168

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@8168

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@8168

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@8168

@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@8168

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@8168

commit: 6a52ad2

[shulhi]

Hmm, any reason you went with one new node to represent two different things? If feel like the optional expression is not the cleanest way to model this. Would it not make more sense to just split these? Is this to remain consistent with something?

I had it split initially, but thought it would be better to represent this as a single node to be consistent with record update.

  | Pexp_record of expression record_element list * expression option
    (* { l1=P1; ...; ln=Pn }     (None)
       { E0 with l1=P1; ...; ln=Pn }   (Some E0) *)

but, you could also make a counter argument with Pexp_field vs Pexp_setfield.

Splitting Pexp_index into two shouldn't be too much work if that is more desired.

[cristianoc]

This is not currently changing the parser. Guess wip. Just, it means it's not been tested yet.

[nojaf]

if that is more desired.

It was just my initial unfiltered response. With records it feels different somehow: { myRec with X } the with X is a clear child node. But for assignment in a[0] = x, it is tagged on and thus not quite an optional extension.

[shulhi]

Okay this is probably closer to record construction vs mutable record field assignment (two different nodes) rather than record update.

This is still in draft though, it is still subject to change and I don't mind changing it to two different nodes.

[zth]

I also think 2 separate nodes are cleaner for this.

Great work btw, this will be a good cleanup/enhancement.