Adding function information to the /render page #134

AlonBilman · 2025-03-19T14:52:32Z

Hey Tamir!

We've added function info to the render page - it updates only when fetching from GitHub (via URL).
Currently, most of the code is in App.svelte.
Here are some example links to test locally (port 5173):
1
2
3
4
5
6
7
8
We are waiting for your feedback and hopefully it meets your standards.

…ith github repos

added css

tmr232

Hi Alon,

Thanks for your contribution!

I went over it, and there are a few things that need more work.
There's a bit of code to change and move around, and tests that need to be written.

Additionally, I think there are visual-design changes required:

The current color is shared for both the dark and light themes, and only kinda works with either. I think it is better to use different colors for the different themes. This is especially important on the light theme, that currently has very low contrast of text and background.
As this is a programming tool, and we're presenting data, I think a monospaced font will be a better fit.
There is currently a lot of spacing in the display. Between the lines and between the labels and the info. I think it should be tightened.
I think the text should be selectable. There is no reason to prevent selection.
I think it'll be better for the data to be presented under the controls and not on the opposite side
Maybe it's worth it to add the project and filepath as well as the function name?

If you have any disagreement regarding the visual side - please let me know!

Last but not least - please make sure you run the linters (bun lint) and they pass before submitting your code. Note that this is critical, and in most cases I would not review code that does not pass the automated checks.

CHANGELOG.md

src/render/src/app.css

src/render/src/App.svelte

src/render/src/app.css

AlonBilman · 2025-03-20T19:59:28Z

Hey Tamir!

First of all, thank you for reviewing the code!

Regarding linting:
When running bun lint, it modifies the entire codebase, resulting in 100+ changes.
Is there a way to apply the linter to a specific file? Running bun lint <file path> also seems to modify everything.

Secondly, in CONTRIBUTING.md, there is a mention of the command bun check but I couldn’t get it to work. It says there is no script called "check."
Could you please clarify how to run this command correctly? - I believe this is the reason the code didn't pass the automation.

Regarding the visual aspects - any preferences you have will be applied

Of course, all your comments will be addressed, fixed, and modified accordingly.

Thanks again for the review! changes are on the way :)

tmr232 · 2025-03-20T20:22:59Z

Regarding linting: When running bun lint, it modifies the entire codebase, resulting in 100+ changes. Is there a way to apply the linter to a specific file? Running bun lint <file path> also seems to modify everything.

The linters passing for all files are required for merging the code, you can see the failing checks at https://github.com/tmr232/function-graph-overview/actions/runs/13949594615/job/39061318568?pr=134
If you're getting a large number of changes, especially in files you did not edit yourself, it is likely that your tools are not up to date as I recently replaced prettier with biome. Try running bun install again and then bun lint.

Secondly, in CONTRIBUTING.md, there is a mention of the command bun check but I couldn’t get it to work. It says there is no script called "check." Could you please clarify how to run this command correctly? - I believe this is the reason the code didn't pass the automation.

That's my bad - I changed the command from bun check to bun lint and forgot to update the docs.

… functions pure.

extracted the functions and changed app.svelte still working on fetching once in render func.

…nName function

…or debugging

…pport for additional node types checked with github urls

# Conflicts: # src/control-flow/function-utils.ts

…oads and destructors Added tests for these cases. bun test: added snapshots (is that ok?)

…ort variable declarations

Bennahmias · 2025-04-24T16:19:08Z

Hey Tamir,

I saw that you changed the test framework from bun:test to vitest, and updated only the src/parser-loader/bun.ts file — but not src/parser-loader/vite.ts.

I wanted to ask if you're planning to update vite.ts as well, because in src/render/src/App.svelte we’re importing initParsers and iterFunctions from file-parsing/vite.ts.

In our tests, we also import from file-parsing/vite.ts, and it’s currently failing because of the wasm path issue. When we import from file-parsing/bun.ts, it works.

Just wanted to check if you're planning to handle this too. Thanks!

tmr232 · 2025-04-24T18:14:49Z

I guess the recent changes make the naming less clear...
vite.js is for the built files (the extension and the web version), bun.js is for scripts and tests.
I might merge those into a single import at some point, but don't know when that'll be.

Used a direct Tree-sitter query Still need to figure out a way to do the same for Go , I couldn't find a generic approach yet.

# Conflicts: # src/test/functionDetails.test.ts

…pdate usages test: add test for function expression with direct name in TypeScript

…ach (except for C++) fix: Go name extraction now works correctly and without bugs all name extraction logic is now working well

…ser zoom

Updated `updateMetadata` to support new logic We should note that I'm still fetching twice from GitHub..

AlonBilman · 2025-05-10T10:03:51Z

Hi Tamir,

Here’s what we’ve done so far:

First, we added support for displaying both the CFG metadata and the function name in GitHub, as well as the CFG metadata in the Graph.

We used Tree-sitter queries to improve the extraction process (thanks for the suggestion).

The code for extracting function names is spread across src/control-flow/, where each cfg-<language>.ts file handles its own logic.

We also kept function-utils, which handles shared logic and dispatching based on language.

About conflicts in function names:
When Tree-sitter queries return multiple matches, we treat it as a conflict and return <unsupported>. This is handled by extractTaggedValueFromTreeSitterQuery.
(These cases are hard to predict and usually don’t result in a valid function name.)

We added 35 tests for all the scenarios we came up with, covering different languages and function types.

On the frontend side:
In App.svelte, we updated the updateMetadata function to support Graph as well, and now we're extracting the metadata based on the render type.

(Just a note: there’s currently nothing in the README that explains how to use Graph on the render page)

When the type is GitHub, we’re fetching the data twice...
Obviously, that’s not ideal. How would you handle it better?

A GIF that summarizes the changes:

AlonBilman · 2025-05-10T10:17:41Z

Also, when I commit my changes, should I also include the snapshot updates that were generated by the tests?

tmr232 · 2025-05-10T10:25:54Z

Also, when I commit my changes, should I also include the snapshot updates that were generated by the tests?

Are you seeing any changes beyond line-ending changes in the snapshots?

AlonBilman · 2025-05-10T10:57:18Z

Also, when I commit my changes, should I also include the snapshot updates that were generated by the tests?

Are you seeing any changes beyond line-ending changes in the snapshots?

Oh, this is weird. Earlier this week, I saw a lot of changes in the snapshot files, but now, when I run the tests, it only changes the end-of-line characters, as you mentioned.
It wasn't like that - you can see it in this commit, where I deleted the changes manually.

But for now, I guess it's alright.

tmr232

Only reviewed the name extraction logic, not the UI part.

There are some questions, but also some bugs. I'm pusing a couple of tests that demonstrate them.

tmr232 · 2025-05-13T17:55:19Z

src/control-flow/cfg-c.ts

@@ -166,3 +166,17 @@ function processSwitchlike(switchSyntax: SyntaxNode, ctx: Context): BasicBlock {

  return blockHandler.update({ entry: headNode, exit: mergeNode });
 }
+
+const nodeType = {


What is this constant for? Why not use the names directlu?

I'll modify it

tmr232 · 2025-05-13T17:57:04Z

src/control-flow/cfg-c.ts

+
+export function extractCFunctionName(func: SyntaxNode): string | undefined {
+  if (func.type === nodeType.functionDefinition) {
+    return func.descendantsOfType(nodeType.identifier)[0]?.text;


Is there a case where this really is undefined, or is it due to the weird typing in the current web-tree-sitter library? If it's the weird types, we have treeSitterNoNullNodes to deal with that in an "easy to find later" way.

tmr232 · 2025-05-13T18:03:11Z

src/control-flow/function-utils.ts

+  tag: string,
+): string | undefined {
+  const language = func.tree.language;
+  const queryObj = new Query(language, query);


I'm pretty sure we can accidentally get a nested function here.
Also, there's an entire query mechanism that we can use.

tmr232 · 2025-05-13T18:04:00Z

src/control-flow/function-utils.ts

+  func: SyntaxNode,
+  language: Language,
+): string | undefined {
+  switch (language) {


Better to use a mapping to easily enforce all languages via typing.

tmr232 · 2025-05-13T18:04:26Z

src/control-flow/function-utils.ts

@@ -0,0 +1,83 @@
+import { Query, type Node as SyntaxNode } from "web-tree-sitter";
+import type { Language } from "./cfg";
+import { extractCFunctionName } from "./cfg-c.ts";


This leads to a cyclic dependency. Honestly - I'm surprised this works at all. In any case - it's better not to have cycles.

tmr232 · 2025-05-13T18:20:07Z

src/control-flow/cfg-go.ts

+  tag: "var.name",
+};
+
+const assignmentQueryAndTag = {


All three queries can be unified with an alternation in the query.

tmr232 · 2025-05-13T18:27:06Z

src/control-flow/cfg-go.ts

+      return extractNameByNodeType(func, nodeType.fieldIdentifier);
+    case nodeType.funcLiteral:
+      // Check if the func_literal is assigned to a variable or is a standalone function
+      return (


It is important to limit the depth of a query, so that they properly ignore nesting.
Consider:

func main() { var x = func() { y := func() {} y() } x() }

See maxStartDepth (used in block-matcher.ts)

You're right. I'll check it out and make the changes.

tmr232 · 2025-05-13T18:33:06Z

src/control-flow/cfg-typescript.ts

+
+const variableDeclaratorQueryAndTag = {
+  query: `
+    (lexical_declaration


What of var definitions, and of definitions without either var, let or const?

tmr232 · 2025-05-13T18:41:30Z

src/control-flow/function-utils.ts

+ *
+ * @param func - The syntax node to search within.
+ * @param type - The type of the child node to extract the name from.
+ * used among all languages (mostly the easy cases of extracting the name).


This line currently renders as part of the parameter definition, which seems odd to me.

tmr232 · 2025-05-13T18:42:21Z

src/control-flow/function-utils.ts

+    .map((c) => c.node.text);
+
+  if (names.length > 1) {
+    return "<unsupported>";


What are we actually checking here? What is not supported, and why are we treating it differently than other cases where we failed to get a name?

As we discussed before, when I have something like y, x := func() {}, func() {}, the query for extracting the variables returns two results:
(short_var_declaration left: (expression_list (identifier) @name))
It's unclear what the "name" of the function is, because there are two expressions. Since I get two matches for one query, I treat it as a conflict.

How can I handle this better?

First thing, adding a comment that says this is helpful.

Second thing - please be consistent. There are more cases where we can't get the name of the function. Why are some just unnamed, and this is <unsupported>?

You're right , I should have included an explanation in the code itself.
Regarding the second thing:
I understand what you're saying. I thought it was different because the extraction succeeded, it just returned more than one name.
In most other cases, I only got a single name, so I could tell if the name extraction had failed.
I could return undefined instead

Should I just return undefined in this case and treat it as an unnamed function?
I think that makes sense

Yep, undefined if we can't get the name, the name if we can, anonymous if there isn't one.

AlonBilman · 2025-05-18T19:31:05Z

Hi ,

So, you’ve added a couple of tests and I’d like to talk about one of them
Let’s look at this code you've given:

func main() {
	var x = func() {
		y := func() {}
		y()
	}
	x()
}

If you ask me what the name of this function is, I’d probably say "main"
Now, when a user gives the render page a GitHub link and a line number that points to a function, they might give the line for func main() { (let’s call that line 1).
If they want to look at the inner function, they would give the line for that nested function instead.
When we fetch the function from GitHub, we only get the function itself. So if we want main, we get everything inside it.
But if we ask for line 2, we only get:

var x = func() {
	y := func() {}
        y()
}

Since we always want to get the outer function name for any link and line number everything works. we would get x.
If we were pointing at y := func() {} , we’d get y.

To be completely honest, I didn’t really understand the test.
Why do we want to iterate inside the node and get the names of nested functions? I think (maybe) it doesn't match the render page feature?
Am I missing something here?

(The test is in Go section)

tmr232 · 2025-05-19T04:42:40Z

If you ask me what the name of this function is, I’d probably say "main"

That may be true, but what the test is asking is "what are the names of all the functions here".

If we have a function that extracts function names, there is no reason for it to not work on nested functions.

The test is there because in that code, I expect to be able to query the names of all the functions, and get correct results (or undefined if we can't get that).
But instead, we get main for main, y for x and y for y.

Additionally, the test does not have to match the /render page. It has to test the function, which should work in any relevant use-case.

AlonBilman · 2025-05-19T05:17:40Z

The test is there because in that code, I expect to be able to query the names of all the functions, and get correct results (or undefined if we can't get that).
But instead, we get main for main, y for x and y for y.

This is kind of a new feature, extracting function names from within a function.
Ok, it still seems a bit odd to me that it doesn't work when we're iterating over the subnodes (right?).
There’s probably a difference here.

Let’s say I have a solution that returns a list of names:
For the render page - yeah, the first one is fine, Id always return the outer name.
The rest would be listed in the order they were declared inside the function?
I guess that makes sense.

I'll keep in touch

tmr232 · 2025-05-20T17:31:43Z

src/control-flow/function-utils.ts

+  const language = func.tree.language;
+  const queryObj = new Query(language, query);
+
+  const rootNode = func.tree.rootNode;


I somehow missed this... You're getting the root of the file - this will never work.

Try the following:

https://github.com/tmr232/function-graph-overview/blob/main/src/control-flow/cfg-python.ts#L359

If you print all the names you get with this, you'll see what I mean.

tmr232 · 2025-05-20T17:33:14Z

Let’s say I have a solution that returns a list of names: For the render page - yeah, the first one is fine, Id always return the outer name. The rest would be listed in the order they were declared inside the function? I guess that makes sense.

There's no need to get a list of names. You need to get the correct name for the function object.

Also, for the render page, it is not always the "outer name", it's the name of the function on the line we choose.
Like I said before, we analyze the entire file.
See the example in #134 (comment)

AlonBilman added 4 commits March 18, 2025 13:02

Added new metadata type, extracted data from cfg and the function.

f7b084e

extracting all the data for issue tmr232#94 , tested the extraction w…

3151ea6

…ith github repos

added the metadata to the page

ca084e1

added css

Updated .md files (changelog and readme)

7a7b13f

tmr232 changed the title ~~Issue#94~~ Adding function information to the /render page Mar 19, 2025

tmr232 self-requested a review March 19, 2025 18:36

tmr232 requested changes Mar 19, 2025

View reviewed changes

linting- commit in order to merge

2ae4d60

AlonBilman closed this Mar 23, 2025

AlonBilman deleted the issue#94 branch March 23, 2025 13:50

AlonBilman restored the issue#94 branch March 23, 2025 14:04

tmr232 reopened this Mar 23, 2025

Bennahmias and others added 16 commits March 26, 2025 14:22

check and update lint

d5bbffb

Trying to find a way to avoid fetching twice and to make the relevant…

a2ba7d5

… functions pure.

Merge remote-tracking branch 'origin/main' into issue#94

7618ad8

Added function name extraction in function-utils.ts

d675eac

extracted the functions and changed app.svelte still working on fetching once in render func.

Add: functionDetils.test.ts file that test for now the extractFunctio…

6e6d675

…nName function

Merge remote-tracking branch 'origin/issue#94' into issue#94_testing

e944b01

Fix: update import path for extractFunctionName and add console log f…

a71484b

…or debugging

Refactor function name extraction to improve type handling and add su…

34e3ae6

…pport for additional node types checked with github urls

Fixed a bug in the TypeScript generator's function name extraction.

328d86d

added documentation

dcea710

Refactor: update tests for extractFunctionName

69bf1a4

Merge remote-tracking branch 'origin/issue#94' into issue#94_testing

0d3f03e

# Conflicts: # src/control-flow/function-utils.ts

simplify anonymous function naming

c279feb

change the test suites

fac3bef

Merge remote-tracking branch 'origin/issue#94' into issue#94_testing

ce19dd5

fix: add some test cases

4d7f20f

AlonBilman added 3 commits April 23, 2025 14:41

Merge branch 'main' into issue#94

1768167

feat(parser): enhance C++ function name extraction for operator overl…

c17b84c

…oads and destructors Added tests for these cases. bun test: added snapshots (is that ok?)

test: add tests for extracting function names from Go literals and sh…

52c72f7

…ort variable declarations

refactor(tests): migrate from bun to vitest and update imports

8c74c99

AlonBilman added 11 commits April 24, 2025 21:29

extract var name from function assignment using Tree-sitter

89a9f51

Used a direct Tree-sitter query Still need to figure out a way to do the same for Go , I couldn't find a generic approach yet.

Merge remote-tracking branch 'origin/issue#94_testing' into issue#94

051ccc6

# Conflicts: # src/test/functionDetails.test.ts

refactor: rename extractNameByNodeName to extractNameByNodeType and u…

8adbcfd

…pdate usages test: add test for function expression with direct name in TypeScript

refactor: replaced findNameInParentHierarchy with a query-based appro…

9880873

…ach (except for C++) fix: Go name extraction now works correctly and without bugs all name extraction logic is now working well

delete snapshot changes. (will be added if needed)

d145111

Improved render style to prevent sliding window from breaking on brow…

3ab9247

…ser zoom

feat: add Graph rendering with CFG-metadata options only

009f53a

Updated `updateMetadata` to support new logic We should note that I'm still fetching twice from GitHub..

Merge remote-tracking branch 'origin/main' into issue#94

e6c8172

CHANGELOG with new features

607b7fe

Revert unnecessary changes. mb.

2469d6d

Revert unnecessary changes. MB.

41ac697

AlonBilman requested a review from tmr232 May 10, 2025 09:56

Merge remote-tracking branch 'origin/main' into issue#94

fd7aa24

tmr232 reviewed May 13, 2025

View reviewed changes

Added a few failing tests that should pass.

421878f

tmr232 reviewed May 20, 2025

View reviewed changes

Adding function information to the /render page #134

Are you sure you want to change the base?

Adding function information to the /render page #134

Uh oh!

Conversation

AlonBilman commented Mar 19, 2025

Uh oh!

tmr232 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlonBilman commented Mar 20, 2025

Uh oh!

tmr232 commented Mar 20, 2025

Uh oh!

Bennahmias commented Apr 24, 2025

Uh oh!

tmr232 commented Apr 24, 2025

Uh oh!

AlonBilman commented May 10, 2025

Uh oh!

AlonBilman commented May 10, 2025

Uh oh!

tmr232 commented May 10, 2025

Uh oh!

AlonBilman commented May 10, 2025

Uh oh!

tmr232 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlonBilman commented May 18, 2025

Uh oh!

tmr232 commented May 19, 2025

Uh oh!

AlonBilman commented May 19, 2025

Uh oh!

tmr232 left a comment •

edited

Loading