Handle absolute paths in typegen #7104

Bushuo · 2024-10-16T20:58:25Z

Like discussed in rescript-lang/rewatch#102 this PR enables gentype to handle passing of absolute file paths.
Instead of manipulating the path to the .cmt file it gets the source file path from the .cmt file first.
Then constructs the output file paths from there.

cristianoc · 2024-10-17T05:04:52Z

Would you do make format and then run tests locally:

make test
make test-gentype

The last command should produce no difference in the output.

Bushuo · 2024-10-17T06:55:22Z

@cristianoc Yes, will do. Tests are failing locally. I will keep working on it.

Bushuo · 2024-10-17T12:29:25Z

@cristianoc I added the resolved project_root and bsb_project_root to the default config.
They where empty strings before.

cristianoc

some comments

cristianoc · 2024-10-17T16:07:26Z

compiler/gentype/GenTypeConfig.ml

@@ -105,6 +105,26 @@ let set_debug ~gtconf =
 let compiler_config_file = "rescript.json"
 let legacy_compiler_config_file = "bsconfig.json"

+let default_config ~project_root ~bsb_project_root =
+  {


{ default with project_root; bsb_project_root }

I made it into a local function instead. with the syntactic improvement.

compiler/gentype/GenTypeMain.ml

cristianoc

I think it's pretty much ready to go, modulo a couple of comments.
And, some testing with rewatch: @mununki

compiler/gentype/Paths.ml

cristianoc · 2024-10-18T04:34:29Z

compiler/gentype/Paths.ml

@@ -29,17 +29,43 @@ let find_name_space cmt =
  cmt |> Filename.basename |> (Filename.chop_extension [@doesNotRaise])
  |> keep_after_dash

-let get_output_file_relative ~config cmt =
-  (cmt |> handle_namespace) ^ ModuleExtension.ts_input_file_suffix ~config
+let remove_project_root_from_absolute_path ~(config : Config.t) source_path =


This is the bit that I haven't really reviewed properly yet.
Might not be a robust way to get a relative path. Especially Windows vs linux etc.
Can't remember if the code already does this somewhere else.
Or, poking around in the Filename module, there's more one can use.

I refactored the function to use Filename.concat for appending the directory separator when missing. Additionally, it now verifies if the prefix exists and returns the original path if not. I also renamed the function for better clarity.

mununki · 2024-10-18T06:31:34Z

This may not be related to this PR, but the genType output for v12 still differs from v11 in this way.

// Title.res
@genType @react.component
let make = (~text) => {
  <div className="title"> {text->React.string} </div>
}

v11

/* TypeScript file generated from Title.res by genType. */

/* eslint-disable */
/* tslint:disable */

import * as TitleJS from './Title.mjs';

export type props<text> = { readonly text: text };

export const make: React.ComponentType<{ readonly text: string }> = TitleJS.make as any;

v12

/* TypeScript file generated from Title.res by genType. */

/* eslint-disable */
/* tslint:disable */

import * as TitleJS from './Title.mjs';

import type {element as Jsx_element} from './Jsx.gen.tsx'; // Error: module not found

export type props<text> = { readonly text: text };

export const make: (_1:props<string>) => Jsx_element = TitleJS.make as any;

cristianoc · 2024-10-18T07:16:09Z

This may not be related to this PR, but the genType output for v12 still differs from v11 in this way.

// Title.res
@genType @react.component
let make = (~text) => {
  <div className="title"> {text->React.string} </div>
}

v11

/* TypeScript file generated from Title.res by genType. */

/* eslint-disable */
/* tslint:disable */

import * as TitleJS from './Title.mjs';

export type props<text> = { readonly text: text };

export const make: React.ComponentType<{ readonly text: string }> = TitleJS.make as any;

v12

/* TypeScript file generated from Title.res by genType. */

/* eslint-disable */
/* tslint:disable */

import * as TitleJS from './Title.mjs';

import type {element as Jsx_element} from './Jsx.gen.tsx'; // Error: module not found

export type props<text> = { readonly text: text };

export const make: (_1:props<string>) => Jsx_element = TitleJS.make as any;

would you open an issue for this
looks like in v12 we expose Jsx types differently, and need to be handled specially by gentype as built in types. Otherwise, as the code generated show, it's just a mysterious unknown type.

mununki · 2024-10-18T08:07:05Z

#7106

cristianoc

Looks ready to go -- would you update the CHAGELOG?

Also, has it been tested on some rewatch project?

zth · 2024-10-19T04:53:54Z

CI seems to fail still.

cristianoc · 2024-10-19T04:59:19Z

CI seems to fail still.

Yes looks like there's a little annotation missing after refactoring to please the static exception checker.

Bushuo · 2024-10-19T05:21:59Z

Nice 🥳 I could swear that the static checker previously ran without having to explicitly make reanalyze.

Bushuo · 2024-10-19T05:26:24Z

Also, has it been tested on some rewatch project?

I only tested the initial version of this PR while I had it based on the v11 branch.
Currently my project fails to build with v12.

cristianoc · 2024-10-19T05:34:35Z

@mununki @zth who has a rewatch project at hand to try this?

zth · 2024-10-19T05:50:33Z

@Bushuo this will need backporting to v11 anyway so if you want you could do a PR on top of this that just backports this to v11 and try it that way.

Bushuo · 2024-10-19T08:35:23Z

@Bushuo this will need backporting to v11 anyway so if you want you could do a PR on top of this that just backports this to v11 and try it that way.

@zth Sure, I can do it. I haven't backported before, though, so if you could give me a quick rundown of the ideal process, that'd be great. I might not get to it today though.

zth · 2024-10-19T08:39:19Z

@Bushuo this will need backporting to v11 anyway so if you want you could do a PR on top of this that just backports this to v11 and try it that way.

@zth Sure, I can do it. I haven't backported before, though, so if you could give me a quick rundown of the ideal process, that'd be great. I might not get to it today though.

Sure! It's really simple, and basically what you did before - just base a separate PR of this work off of 11.0_release instead of master. I think there are some formatting differences to watch out for, but it should be fairly straight forward.

So, after that, you'll have 2 PRs:

This, based on master, that'll go into v12
A separate PR based on 11.0_release, that'll go into the next v11 patch release

Clear?

Bushuo · 2024-10-20T12:43:24Z

@cristianoc @zth
I assumed FindSourceFile.ml cmt cmt_annots would always return an absolute path, allowing it to be used for both output_file and output_file_relative. However, this only seems true for v12, not v11.

On the v11 branch, the function returns test/variantsMatching.res for the file jscomp/test/variantsMatching.res.
I suspect this is related to how sources is configured in bsconfig.json.

The config file is at the root level.

"sources": {
    "dir": "jscomp/test/"
}

In contrast, configuring a similar test in v12 still returns an absolute path.
Do you know why this behavior differs and could you provide some guidance on how to proceed?

cristianoc · 2024-10-20T14:03:00Z

@zth why do we need v11 support?

zth · 2024-10-20T14:10:10Z

@zth why do we need v11 support?

Because it's likely a large portion of users will remain on v11 for a good while. Backporting this will mean that those can move to rewatch if they're using gentype as well before it ships in the compiler itself only.

Bushuo · 2024-10-21T06:26:26Z

I made some changes and now it also seems to work on v11. Could you please take a look again?

cristianoc

Looks good to me.

zth · 2024-10-21T08:48:51Z

@Bushuo great work! ⭐

* refactor: extract fun from process_cmt_file * refactor: read the .cmt earlier * feat: handle paths via source_file instead of cmt_file * add project root to default config * refactor: extract funs to remove duplication * fix: consider file seperators * fix: please static checker * make find source file return only absolute paths * refactor: cleanup getOutputFile and getOutputFileRelative * chore: update changelog

cristianoc reviewed Oct 17, 2024

View reviewed changes

Bushuo force-pushed the gentype-abs-paths branch from 9f29767 to 10a5d70 Compare October 17, 2024 18:11

cristianoc requested a review from mununki October 18, 2024 04:36

cristianoc reviewed Oct 18, 2024

View reviewed changes

cristianoc approved these changes Oct 19, 2024

View reviewed changes

Bushuo mentioned this pull request Oct 20, 2024

v11 backport of #7104 #7111

Merged

Bushuo added 7 commits October 20, 2024 22:21

refactor: extract fun from process_cmt_file

c5dd009

refactor: read the .cmt earlier

678e6d4

feat: handle paths via source_file instead of cmt_file

3accab4

add project root to default config

8d22164

refactor: extract funs to remove duplication

f62fd61

fix: consider file seperators

3b6ad82

fix: please static checker

11f9c42

Bushuo force-pushed the gentype-abs-paths branch from 9759b80 to 5780c0a Compare October 20, 2024 20:22

Bushuo added 3 commits October 20, 2024 22:26

chore: update changelog

6f9d118

make find source file return only absolute paths

8da295e

refactor: cleanup get_output_file and get_output_file_relative

5bb3db9

Bushuo force-pushed the gentype-abs-paths branch from 5780c0a to 5bb3db9 Compare October 20, 2024 20:26

Bushuo requested a review from cristianoc October 21, 2024 06:25

cristianoc approved these changes Oct 21, 2024

View reviewed changes

zth merged commit 47870d0 into rescript-lang:master Oct 21, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle absolute paths in typegen #7104

Handle absolute paths in typegen #7104

Bushuo commented Oct 16, 2024 •

edited

Loading

cristianoc commented Oct 17, 2024

Bushuo commented Oct 17, 2024

Bushuo commented Oct 17, 2024

cristianoc left a comment

cristianoc Oct 17, 2024

Bushuo Oct 17, 2024

cristianoc left a comment

cristianoc Oct 18, 2024

Bushuo Oct 18, 2024

mununki commented Oct 18, 2024

cristianoc commented Oct 18, 2024

mununki commented Oct 18, 2024

cristianoc left a comment

zth commented Oct 19, 2024

cristianoc commented Oct 19, 2024

Bushuo commented Oct 19, 2024

Bushuo commented Oct 19, 2024 •

edited

Loading

cristianoc commented Oct 19, 2024

zth commented Oct 19, 2024

Bushuo commented Oct 19, 2024

zth commented Oct 19, 2024

Bushuo commented Oct 20, 2024

cristianoc commented Oct 20, 2024

zth commented Oct 20, 2024

Bushuo commented Oct 21, 2024

cristianoc left a comment

zth commented Oct 21, 2024

Handle absolute paths in typegen #7104

Handle absolute paths in typegen #7104

Conversation

Bushuo commented Oct 16, 2024 • edited Loading

cristianoc commented Oct 17, 2024

Bushuo commented Oct 17, 2024

Bushuo commented Oct 17, 2024

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc Oct 17, 2024

Choose a reason for hiding this comment

Bushuo Oct 17, 2024

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc Oct 18, 2024

Choose a reason for hiding this comment

Bushuo Oct 18, 2024

Choose a reason for hiding this comment

mununki commented Oct 18, 2024

cristianoc commented Oct 18, 2024

mununki commented Oct 18, 2024

cristianoc left a comment

Choose a reason for hiding this comment

zth commented Oct 19, 2024

cristianoc commented Oct 19, 2024

Bushuo commented Oct 19, 2024

Bushuo commented Oct 19, 2024 • edited Loading

cristianoc commented Oct 19, 2024

zth commented Oct 19, 2024

Bushuo commented Oct 19, 2024

zth commented Oct 19, 2024

Bushuo commented Oct 20, 2024

cristianoc commented Oct 20, 2024

zth commented Oct 20, 2024

Bushuo commented Oct 21, 2024

cristianoc left a comment

Choose a reason for hiding this comment

zth commented Oct 21, 2024

Bushuo commented Oct 16, 2024 •

edited

Loading

Bushuo commented Oct 19, 2024 •

edited

Loading