Skip to content
This repository was archived by the owner on Sep 9, 2025. It is now read-only.

fix: ignore .ts and .js extensions form imports #437

Merged
hendrikvanantwerpen merged 3 commits into from
May 31, 2024
Merged

fix: ignore .ts and .js extensions form imports #437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d28315
fix: ignore .ts and .js extensions form imports
nohehf May 27, 2024
d0b940e
fix: add auxiliary function to remove file ext
nohehf May 27, 2024
c43fa8b
feat: use replace instead of custom function
nohehf May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions languages/tree-sitter-stack-graphs-typescript/src/stack-graphs.tsg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ attribute node_symbol = node => symbol = (source-text node), source_n

; expose globals
edge proj_scope -> @prog.globals

Comment thread
nohehf marked this conversation as resolved.
Outdated
var mod_scope = proj_scope
if (not (is-empty @imexs)) {
; module definition
Expand Down Expand Up @@ -541,7 +541,9 @@ attribute node_symbol = node => symbol = (source-text node), source_n
; module reference
var mod_scope = mod_ref__ns
scan (path-normalize mod_path) {
"([^/]+)/?" {
; ignore .js and .ts extensions on imports, as ts allows this
; this might lead to false positives
"([^/|(\.j|ts)]+)/?" {
Copy link
Copy Markdown
Contributor

@hendrikvanantwerpen hendrikvanantwerpen May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is correct, bit this doesn't look quite right. What about this?

Suggested change
"([^/|(\.j|ts)]+)/?" {
"([^/]+)(/|\.ts|\.tsx|\.js|\.jsx\)?" {

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my regex is a bit of a hack but as .js (and .ts, ...) are included in this broad pattern: ([^/]+) it will be included in the first group if this is optional: (/|\.ts|\.tsx|\.js|\.jsx\) (eg. it will still match the extention). Working on something cleaner.

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding lazy matching with ? in the first group seems to fix the issue:
^([^/]+?)(\.js|\.ts|\.jsx|\.tsx)?$seems ok, updating the pr

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the regex parser does not support non greedy matching ? (...?)

Copy link
Copy Markdown
Contributor

@hendrikvanantwerpen hendrikvanantwerpen May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the problem! An alternative might be to keep the original regex, and strip any of the extensions off inside the body using (replace ...). That might be easier.

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tried a way using regexes and replace, but it seems the regex in tsg does not strictly behave as in rust:
Given the regex \.[^\.\\/]+$

scan "./bar" {
    "\.[^\.\\/]+$" {
      print $0
    }
 }

Outputs /bar
But using the same regex in rust it does not match (as expected):
https://regex101.com/r/Rwm8lq/1 (check the unit tests tab)
(also tested on https://rustexp.lpil.uk/)

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the implementation of path-dir is kind of wrong:

functions.add(
    "path-dir".into(),
    path_fn(|p| p.parent().map(|s| s.as_os_str().to_os_string())),
);

For instance I expect the dir of . to be . and the dir of .. to be .., but their parents are both None.

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it to work by removing the explicit extensions (.js, .ts, .jsx, .tsx, not all extensions) with: (path-normalize (replace mod_path "\.(js|ts|jsx|tsx)$" "")), but the questions above still remain and should be considered I believe

Copy link
Copy Markdown
Contributor Author

@nohehf nohehf May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(tho this can be merged I believe)

Copy link
Copy Markdown
Contributor

@hendrikvanantwerpen hendrikvanantwerpen May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My kingdom for a small coherent theory of paths 😭. Thanks for figuring out these cases. I'll put them in an issue so we document this in a more visible place.

node mod_ref
attr (mod_ref) push_symbol = $1
edge mod_ref -> mod_scope
Expand Down
6 changes: 6 additions & 0 deletions languages/tree-sitter-stack-graphs-typescript/test/modules/import-extension-js.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* --- path: src/foo.ts --- */
export const bar = 42;

/* --- path: src/index.ts --- */
import { bar } from "./foo.js";
// ^ defined: 2
6 changes: 6 additions & 0 deletions languages/tree-sitter-stack-graphs-typescript/test/modules/import-extension-none.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* --- path: src/foo.ts --- */
export const bar = 42;

/* --- path: src/index.ts --- */
import { bar } from "./foo";
// ^ defined: 2
6 changes: 6 additions & 0 deletions languages/tree-sitter-stack-graphs-typescript/test/modules/import-extension-ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* --- path: src/foo.ts --- */
export const bar = 42;

/* --- path: src/index.ts --- */
import { bar } from "./foo.ts";
// ^ defined: 2