Remove `res.namedArgLoc` attribute and store the location information directly into the label. #7247

cristianoc · 2025-01-14T12:04:22Z

Instead of the attribute res.namedArgLoc, there's now Asttypes.arg_label_loc to store labels with location.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite	Current: `a47a3cb`	Previous: `e1b7fb7`	Ratio
`Parse RedBlackTree.res - time/run`	`1.3690002266666665` ms	`1.2123143266666667` ms	`1.13`
`Parse Napkinscript.res - time/run`	`42.84875455333333` ms	`39.28006235333333` ms	`1.09`
`Parse HeroGraphic.res - time/run`	`5.7314326866666665` ms	`5.13472718` ms	`1.12`

This comment was automatically generated by workflow using github-action-benchmark.

nojaf · 2025-01-21T16:11:08Z

So, for my understanding, this moves the location from @res.namedArgLoc to named argument node?

cristianoc · 2025-01-21T19:06:37Z

So, for my understanding, this moves the location from @res.namedArgLoc to named argument node?

Yes. It's been used in different ast nodes. The current state of the PR covers a subset of them.

nojaf · 2025-01-24T13:46:02Z

analysis/src/CompletionFrontEnd.ml

@@ -1444,10 +1447,10 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
                     functionContextPath = ctxPath;
                     argumentLabel =
                       (match lbl with
-                       | Nolabel ->
+                       | Nolbl ->


Sorry to nitpick, but is there any rational for these shorter names?
As a novice to all this, I'd say it make it all a bit more obscure.
But perhaps this has a reason.

No special plan for naming at the moment.
The situation is there's an existing variant type, without locations on labels.
Here I have introduced a new variant type, with locations.
The old type still exists, as labels are used e.g. in Tarrow in types.mli, and those are generated by the compiler without reference to any source location (e.g. via unification or other reasons).

So for now I have different variant names for the new type definition, as there are quite a few places where the type is inferred by the name.
One could have 2 types with identical variants names, though that might actually add more confusion (and need to explicitly add more type annotations to indicate which type is intended).
One could try to make the payload of labelled case a type variable, to be instantiated with either string or a string plus location depending on context. I haven't tried this, not sure how that would look.

One could also use a single type and stuff a lot of Location.none when that type is not supposed to have a location. Not particularly keen on that as it becomes unclear where to expect a location and where not.

Here is how things look when using identical variant names for the two types: with and without locations: #7254

One could have 2 types with identical variants names, though that might actually add more confusion (and need to explicitly add more type annotations to indicate which type is intended).

I also think this probably causes more confusion.

Another idea might be to use some prefix for disambiguation, similar to Pexp_xxx vs. Texp_xxx etc. Like Lloc_label / Lloc_nolabel vs. the existing Label / Nolabel.

One could have 2 types with identical variants names, though that might actually add more confusion (and need to explicitly add more type annotations to indicate which type is intended).

I also think this probably causes more confusion.

Another idea might be to use some prefix for disambiguation, similar to Pexp_xxx vs. Texp_xxx etc. Like Lloc_label / Lloc_nolabel vs. the existing Label / Nolabel.

I've tried putting the definition without location inside an inner module Noloc (and leave the rest as is).
Only a few changes needed to add Noloc. prefix..
It might be the lowest friction way, and the definition with loc can probably be renamed back to the original name hopefully with no more changes. How does it sound?

Sounds great!

Sounds great!

OK done, the renaming worked.

cknitt · 2025-01-28T18:28:32Z

tests/syntax_tests/data/ppx/react/expected/externalWithCustomName.res.txt

@@ -8,7 +8,7 @@ module Foo = {
  external component: React.componentLike<props<int, string>, React.element> = "component"
 }

-let t = React.createElement(Foo.component, {a: 1, b: "1"})
+let t = React.createElement(Foo.component, {a: 1, b: {"1"}})


Is this test output change ok/expected?

Note that this is not a printing test, but shows the result of the jsx transform. So it aims to show the effect of the transformation.
That said, the braces { were present in the source, so it's likely that these are preserved better now because there are no res.namedArgLoc attributes around to confuse the printing logic.

cknitt · 2025-01-28T18:31:04Z

tests/syntax_tests/data/ppx/react/expected/uncurriedProps.res.txt

@@ -51,7 +51,7 @@ module Bar = {
  @res.jsxComponentProps
  type props = {}

-  let make = (_: props) => React.jsx(Foo.make, {callback: (_, _, _) => ()})
+  let make = (_: props) => React.jsx(Foo.make, {callback: {(_, _, _) => ()}})


And this one?

same comment as for the other ppx test

cknitt · 2025-01-28T18:32:35Z

tests/syntax_tests/data/ppx/react/expected/forwardRef.res.txt

@@ -21,7 +21,7 @@ module V4C = {
            ~props={
              type_: "text",
              ?className,
-              ref: ?(Js.Nullable.toOption(ref)->Belt.Option.map(React.Ref.domRef)),
+              ref: ?{Js.Nullable.toOption(ref)->Belt.Option.map(React.Ref.domRef)},


And this one?

same comment as for the other ppx test

cristianoc · 2025-01-30T09:09:23Z

This should be good to go, after checking on a real project. Though the changes should only be about formatting, as label location does not affect compilation.

cknitt · 2025-01-30T11:58:46Z

Also tested an LGTM. Needs a rebase now because of the CHANGELOG though.

…he AST.

Preserve `isMobile={isMobile}` when it's like that in the source, instead of reformatting to `isMobile`. Do we want that?

…ate field.

cknitt

Thanks!

cristianoc force-pushed the named-arg-loc-fundef branch 3 times, most recently from 21d50c8 to be89110 Compare January 21, 2025 15:42

github-actions bot reviewed Jan 21, 2025

View reviewed changes

cristianoc mentioned this pull request Jan 24, 2025

Store the label loc directly in the label, for application for now. #7253

Merged

cristianoc changed the title ~~Experiment with storing the location of function named arguments in t…~~ Remove res.namedArgLoc attribute and store the location information directly into the label. Jan 24, 2025

nojaf reviewed Jan 24, 2025

View reviewed changes

cristianoc requested review from cknitt and zth January 24, 2025 15:37

cknitt reviewed Jan 28, 2025

View reviewed changes

cristianoc mentioned this pull request Jan 29, 2025

Use identical variant names for arg_label and arg_label_loc. #7254

Closed

cristianoc force-pushed the named-arg-loc-fundef branch 3 times, most recently from 0656d1d to 8697ecc Compare January 30, 2025 08:47

cristianoc force-pushed the named-arg-loc-fundef branch from 8697ecc to 1a3869f Compare January 30, 2025 09:49

cristianoc added 9 commits January 30, 2025 14:28

Experiment with storing the location of function named arguments in t…

7c2b435

…he AST.

Store label location in type argument instead of attribute.

60cb16e

Store the label loc directly in the label, for application for now.

cbfc817

restore some punning behaviour

89a998b

Preserve `isMobile={isMobile}` when it's like that in the source, instead of reformatting to `isMobile`. Do we want that?

Store location directly in the label on Ptyp_arrow instead of a separ…

1b83538

…ate field.

Store location directly in the label on Pexp_fun.

8a00b56

Put type without locs inside module Noloc.

69dee38

Restore original variant names.

3c0760b

Update CHANGELOG.md

a47a3cb

cristianoc force-pushed the named-arg-loc-fundef branch from 1a3869f to a47a3cb Compare January 30, 2025 13:29

cknitt approved these changes Jan 30, 2025

View reviewed changes

cknitt merged commit afa723f into master Jan 30, 2025
19 of 20 checks passed

cknitt deleted the named-arg-loc-fundef branch January 30, 2025 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `res.namedArgLoc` attribute and store the location information directly into the label. #7247

Remove `res.namedArgLoc` attribute and store the location information directly into the label. #7247

cristianoc commented Jan 14, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

nojaf commented Jan 21, 2025 •

edited

Loading

cristianoc commented Jan 21, 2025

nojaf Jan 24, 2025

cristianoc Jan 24, 2025

cristianoc Jan 24, 2025

cristianoc Jan 25, 2025

cknitt Jan 26, 2025

cristianoc Jan 28, 2025

cknitt Jan 28, 2025

cristianoc Jan 28, 2025

cknitt Jan 28, 2025

cristianoc Jan 29, 2025

cknitt Jan 28, 2025

cristianoc Jan 29, 2025

cknitt Jan 28, 2025

cristianoc Jan 29, 2025

cristianoc commented Jan 30, 2025

cknitt commented Jan 30, 2025

cknitt left a comment

Remove res.namedArgLoc attribute and store the location information directly into the label. #7247

Remove res.namedArgLoc attribute and store the location information directly into the label. #7247

Conversation

cristianoc commented Jan 14, 2025 • edited Loading

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

nojaf commented Jan 21, 2025 • edited Loading

cristianoc commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jan 30, 2025

cknitt commented Jan 30, 2025

cknitt left a comment

Choose a reason for hiding this comment

Remove `res.namedArgLoc` attribute and store the location information directly into the label. #7247

Remove `res.namedArgLoc` attribute and store the location information directly into the label. #7247

cristianoc commented Jan 14, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

nojaf commented Jan 21, 2025 •

edited

Loading