Skip to content

Commit bfbc9ee

Browse files
Fix GenType path resolution for Windows short paths and POSIX symlinks (#8639)
* Fix GenType path resolution on Windows Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Add changelog entry Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Test GenType project roots Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Run GenType path test through Windows shim Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Exercise GenType through a Windows short path Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Use a numbered wrapper for the GenType test Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Group the GenType integration test Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Fix GenType path test on CI Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Tighten GenType project path regression coverage Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> * Preserve GenType editor paths and test Windows root traversal Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com> --------- Signed-off-by: Florian Hammerschmidt <florianh89@gmail.com>
1 parent ef72591 commit bfbc9ee

10 files changed

Lines changed: 209 additions & 23 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Fix GenType path resolution for Windows short paths and POSIX symlinked project paths, and prevent traversal from hanging at a filesystem root. https://github.com/rescript-lang/rescript/pull/8639

‎compiler/gentype/module_resolver.ml‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,12 @@ let create_lazy_resolver ~config ~extensions ~exclude_file =
109109
let apply ~resolver ~use_bs_dependencies module_name =
110110
module_name |> Lazy.force resolver.lazy_find ~use_bs_dependencies
111111

112+
let rec path_to_list path =
113+
let parent = Filename.dirname path in
114+
(* On Windows, the basename of a drive root omits the drive prefix. *)
115+
if Filename.basename path = path || parent = path then [path]
116+
else (path |> Filename.basename) :: path_to_list parent
117+
112118
(** Resolve a reference to ModuleName, and produce a path suitable for require.
113119
E.g. require "../foo/bar/ModuleName.ext" where ext is ".res" or ".js". *)
114120
let resolve_module ~(config : Config.t) ~import_extension ~output_file_relative
@@ -132,13 +138,6 @@ let resolve_module ~(config : Config.t) ~import_extension ~output_file_relative
132138
in
133139
if Sys.file_exists module_name_res_file then candidate
134140
else
135-
let rec path_to_list path =
136-
let is_root = path |> Filename.basename = path in
137-
match is_root with
138-
| true -> [path]
139-
| false ->
140-
(path |> Filename.basename) :: (path |> Filename.dirname |> path_to_list)
141-
in
142141
match module_name |> apply ~resolver ~use_bs_dependencies with
143142
| None -> candidate
144143
| Some (resolved_module_dir, case, bs_dependencies) ->

‎rewatch/src/build.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ pub fn get_compiler_args(rescript_file_path: &Path) -> Result<String> {
136136
};
137137
let compiler_args = compile::compiler_args(
138138
&project_context.current_config,
139+
&current_package,
139140
&ast_path,
140141
relative_filename,
141142
is_interface,

‎rewatch/src/build/compile.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -751,6 +751,9 @@ pub fn get_runtime_path_args(
751751

752752
pub fn compiler_args(
753753
config: &config::Config,
754+
// Match the parser's root spelling: canonical during builds, caller-supplied
755+
// for compiler-args consumers such as editor incremental type checking.
756+
project_root: &Path,
754757
ast_path: &Path,
755758
file_path: &Path,
756759
is_interface: bool,
@@ -833,7 +836,10 @@ pub fn compiler_args(
833836
};
834837

835838
let package_name_arg = vec!["-bs-package-name".to_string(), config.name.to_owned()];
836-
let project_root_args = config.get_project_root_args();
839+
let project_root_args = vec![
840+
"-bs-project-root".to_string(),
841+
project_root.to_string_lossy().to_string(),
842+
];
837843

838844
let implementation_args = if is_interface {
839845
debug!("Compiling interface file: {}", &module_name);
@@ -1022,6 +1028,7 @@ fn compile_file(
10221028
let current_package_dirs: &[PathBuf] = package.gentype_dirs.as_deref().unwrap_or(&[]);
10231029
let to_mjs_args = compiler_args(
10241030
&package.config,
1031+
&package.path,
10251032
ast_path,
10261033
implementation_file_path,
10271034
is_interface,

‎rewatch/src/config.rs‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,20 +1009,6 @@ impl Config {
10091009
args
10101010
}
10111011

1012-
/// Directory containing the `rescript.json` this config was parsed from.
1013-
fn get_package_root(&self) -> &Path {
1014-
self.path
1015-
.parent()
1016-
.expect("rescript.json path should always have a parent directory")
1017-
}
1018-
1019-
pub fn get_project_root_args(&self) -> Vec<String> {
1020-
vec![
1021-
"-bs-project-root".to_string(),
1022-
self.get_package_root().to_string_lossy().to_string(),
1023-
]
1024-
}
1025-
10261012
pub fn get_warning_args(&self, is_local_dep: bool, warn_error_override: Option<String>) -> Vec<String> {
10271013
// Ignore warning config for non local dependencies (node_module dependencies)
10281014
if !is_local_dep {
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#!/bin/bash
2+
set -euo pipefail
3+
cd "$(dirname "$0")"
4+
source "../utils.sh"
5+
6+
bold "Test: GenType resolves modules through a non-canonical project path"
7+
node ./project-root.mjs
8+
success "GenType project-root checks completed"
Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
import assert from "node:assert/strict";
2+
import {spawnSync} from "node:child_process";
3+
import {existsSync, mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync} from "node:fs";
4+
import {tmpdir} from "node:os";
5+
import {join} from "node:path";
6+
7+
const executable = process.env.REWATCH_EXECUTABLE;
8+
assert(executable, "REWATCH_EXECUTABLE must be set");
9+
10+
const executablePath = executable.endsWith(".exe") ? executable : `${executable}.exe`;
11+
const commandShim = executable.endsWith(".cmd") ? executable : `${executable}.cmd`;
12+
const resolvedExecutable = existsSync(executablePath)
13+
? executablePath
14+
: existsSync(commandShim)
15+
? commandShim
16+
: executable;
17+
18+
function runBuild(buildDirectory) {
19+
if (process.platform !== "win32") {
20+
return spawnSync(executable, ["build", buildDirectory], {
21+
cwd: tmpdir(),
22+
encoding: "utf8",
23+
env: process.env,
24+
timeout: 30_000,
25+
});
26+
}
27+
28+
return spawnSync(
29+
process.env.ComSpec ?? "cmd.exe",
30+
[
31+
"/d",
32+
"/c",
33+
'cd /d "%REWATCH_TEST_PROJECT%" && "%REWATCH_TEST_EXECUTABLE%" build',
34+
],
35+
{
36+
encoding: "utf8",
37+
env: {
38+
...process.env,
39+
REWATCH_TEST_EXECUTABLE: resolvedExecutable,
40+
REWATCH_TEST_PROJECT: buildDirectory,
41+
},
42+
timeout: 30_000,
43+
windowsVerbatimArguments: true,
44+
},
45+
);
46+
}
47+
48+
function getBuildDirectory(projectDir) {
49+
if (process.platform !== "win32") {
50+
const symlink = `${projectDir}-link`;
51+
symlinkSync(projectDir, symlink, "dir");
52+
return symlink;
53+
}
54+
55+
const expandedPath = realpathSync.native(projectDir);
56+
const result = spawnSync(
57+
process.env.ComSpec ?? "cmd.exe",
58+
["/d", "/c", 'for %I in ("%REWATCH_TEST_PROJECT%") do @echo %~sI'],
59+
{
60+
encoding: "utf8",
61+
env: {...process.env, REWATCH_TEST_PROJECT: expandedPath},
62+
windowsVerbatimArguments: true,
63+
},
64+
);
65+
assert.ifError(result.error);
66+
assert.equal(result.status, 0, result.stderr);
67+
68+
const shortPath = result.stdout.trim();
69+
assert(shortPath, "Windows did not return a short path for the temporary project");
70+
if (shortPath.toLowerCase() === expandedPath.toLowerCase()) {
71+
console.log("SKIP: Windows 8.3 short names are unavailable on this volume");
72+
return null;
73+
}
74+
return shortPath;
75+
}
76+
77+
function checkEditorCompilation(buildDirectory) {
78+
const options = {encoding: "utf8", timeout: 30_000};
79+
const result = spawnSync(
80+
resolvedExecutable,
81+
["compiler-args", join(buildDirectory, "src-a", "A.res")],
82+
{...options, shell: resolvedExecutable.endsWith(".cmd")},
83+
);
84+
assert.ifError(result.error);
85+
assert.equal(result.status, 0, result.stderr);
86+
const {compiler_args: args} = JSON.parse(result.stdout);
87+
assert.equal(args[args.indexOf("-bs-project-root") + 1], buildDirectory);
88+
89+
// The editor compiles an absolute incremental source instead of the AST,
90+
// and omits generated-JavaScript output while retaining the GenType flags.
91+
args.splice(args.indexOf("-bs-package-output"), 2);
92+
args.pop();
93+
const incrementalDir = join(buildDirectory, "lib", "bs", "___incremental");
94+
mkdirSync(incrementalDir);
95+
const source = join(incrementalDir, "Editor.res");
96+
writeFileSync(source, "@genType\nlet value = 42\n");
97+
const compiled = spawnSync(process.env.RESCRIPT_BSC_EXE, [...args, source], {
98+
...options,
99+
cwd: join(buildDirectory, "lib", "bs"),
100+
});
101+
assert.ifError(compiled.error);
102+
assert.equal(compiled.status, 0, `${compiled.stdout}\n${compiled.stderr}`);
103+
assert(existsSync(join(incrementalDir, "Editor.gen.tsx")));
104+
}
105+
106+
const projectDir = mkdtempSync(join(tmpdir(), "rescript-gentype-root-"));
107+
108+
try {
109+
mkdirSync(join(projectDir, "src-a"));
110+
mkdirSync(join(projectDir, "src-b", "nested"), {recursive: true});
111+
writeFileSync(
112+
join(projectDir, "rescript.json"),
113+
`${JSON.stringify(
114+
{
115+
name: "gentype-project-root",
116+
sources: ["src-a", {dir: "src-b", subdirs: true}],
117+
gentypeconfig: {},
118+
},
119+
null,
120+
2,
121+
)}\n`,
122+
);
123+
writeFileSync(join(projectDir, "src-a", "A.res"), "@genType\ntype t = {value: int}\n");
124+
writeFileSync(
125+
join(projectDir, "src-b", "nested", "B.res"),
126+
"@genType\nlet getValue = (record: A.t) => record.value\n",
127+
);
128+
129+
const buildDirectory = getBuildDirectory(projectDir);
130+
if (buildDirectory !== null) {
131+
assert(existsSync(buildDirectory), `Build directory does not exist: ${buildDirectory}`);
132+
const result = runBuild(buildDirectory);
133+
134+
assert.ifError(result.error);
135+
assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`);
136+
assert.match(
137+
readFileSync(join(projectDir, "src-b", "nested", "B.gen.tsx"), "utf8"),
138+
/from '\.\.\/\.\.\/src-a\/A\.gen'/,
139+
);
140+
checkEditorCompilation(buildDirectory);
141+
}
142+
} finally {
143+
rmSync(`${projectDir}-link`, {force: true});
144+
rmSync(projectDir, {recursive: true, force: true});
145+
}

‎rewatch/tests/suite.sh‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ fi
137137
./compile/15-no-new-files.sh &&
138138
./compile/16-snapshots-unchanged.sh &&
139139

140+
# GenType tests
141+
./gentype/01-gentype-project-root.sh &&
142+
140143
# Watch tests
141144
./watch/01-watch-recompile.sh &&
142145
./watch/02-watch-warnings-persist.sh &&

‎scripts/test.js‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,29 @@ if (formatTest) {
6565

6666
if (ounitTest) {
6767
if (process.platform === "win32") {
68-
console.log("Skipping OUnit tests on Windows");
68+
// GenType's drive-root regression needs the native Windows Filename module.
69+
const binary = path.join(
70+
projectDir,
71+
"_build/default/tests/ounit_tests/ounit_tests_main.exe",
72+
);
73+
const { stdout } = await execBin(binary, ["-list-test"], {
74+
shell: false,
75+
throwOnFail: true,
76+
});
77+
const tests = stdout
78+
.split(/\r?\n/)
79+
.filter(test => test.includes(":gentype:"));
80+
if (tests.length === 0) {
81+
throw new Error("No GenType unit tests found");
82+
}
83+
await execBin(
84+
binary,
85+
tests.flatMap(test => ["-only-test", test]),
86+
{
87+
stdio: "inherit",
88+
shell: false,
89+
},
90+
);
6991
} else {
7092
await execBin(ounitTestBin, [], {
7193
stdio: "inherit",

‎tests/ounit_tests/ounit_gentype_tests.ml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,20 @@ open OUnit
33
let suites =
44
"gentype"
55
>::: [
6+
( "split relative module paths" >:: fun _ ->
7+
assert_equal ["."] (Module_resolver.path_to_list ".");
8+
assert_equal ["src"] (Module_resolver.path_to_list "src");
9+
assert_equal ["nested"; "src"]
10+
(Module_resolver.path_to_list (Filename.concat "src" "nested")) );
11+
( "stop at Windows drive roots" >:: fun _ ->
12+
skip_if (not Sys.win32) "Windows drive roots require Windows";
13+
List.iter
14+
(fun root ->
15+
assert_equal [root] (Module_resolver.path_to_list root);
16+
assert_equal ["nested"; "src"; root]
17+
(Module_resolver.path_to_list
18+
(Filename.concat root (Filename.concat "src" "nested"))))
19+
["C:\\"; "D:\\"] );
620
( "escape semantic import paths" >:: fun _ ->
721
let emit path =
822
path |> Import_path.from_string_unsafe |> Import_path.emit

0 commit comments

Comments
 (0)