Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(graphql_semantic): build semantic model from AST #3378

Merged

Conversation

vohoanglong0107
Copy link
Contributor

@vohoanglong0107 vohoanglong0107 commented Jul 8, 2024

Summary

Depends on #3427

Build a Semantic model for GraphQL from AST.

Unlike in JS, in GraphQL, a variable can be referenced in a fragment, which in turn can be referenced by multiple operations, and then defined in those operations. This results in a variable being potentially defined in multiple locations.

fragment HeroDetails on Character {
    name(startWith: $start)
}

query HeroWithH($start: String = "H") {
	...HeroDetails
}

query HeroWithO($start: String = "O") {
	...HeroDetails
}

In this case, the $start variable is defined in both the HeroWithH and HeroWithO variable definitions, and referenced at (startWith: $start) in HeroDetails fragment.

To address this, each operation and fragment will be associated with a scope. The scope will keep track of every referenced fragment, allowing it to track every referenced variable, either directly referenced or indirectly through a referenced fragment. Then, it will try to bind every referenced variable to its declaration inside an operation definition.

Test Plan

All the calls to retrieve binding and reference should correctly return the respective bindings and references of the AST node.

@github-actions github-actions bot added the A-Tooling Area: internal tools label Jul 8, 2024
Copy link

codspeed-hq bot commented Jul 8, 2024

CodSpeed Performance Report

Merging #3378 will improve performances by 6.45%

Comparing vohoanglong0107:feat-init-graphql-semantic (dfd03cd) with main (2ef23e7)

Summary

⚡ 1 improvements
✅ 103 untouched benchmarks

Benchmarks breakdown

Benchmark main vohoanglong0107:feat-init-graphql-semantic Change
dojo_11880045762646467684.js[cached] 8.5 ms 8 ms +6.45%

@vohoanglong0107 vohoanglong0107 force-pushed the feat-init-graphql-semantic branch 6 times, most recently from fea9d32 to d0b9435 Compare July 12, 2024 03:32
@vohoanglong0107 vohoanglong0107 marked this pull request as ready for review July 12, 2024 04:01
@vohoanglong0107 vohoanglong0107 changed the title feat(graphql_semantic): build semantic model from CST feat(graphql_semantic): build semantic model from AST Jul 12, 2024
@ematipico
Copy link
Member

Are you able to separate the changes you made to the parser from the actual model changes? Or are they connected? It doesn't seem they are by reading the description of the PR

@vohoanglong0107
Copy link
Contributor Author

Ah sure

@vohoanglong0107 vohoanglong0107 force-pushed the feat-init-graphql-semantic branch 2 times, most recently from 9875121 to 1523f0a Compare July 15, 2024 12:51
@github-actions github-actions bot removed the A-Tooling Area: internal tools label Jul 15, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

This is great!

Comment on lines +119 to +122
enum BindingName {
Type(TokenText),
Value(TokenText),
}
Copy link
Member

Choose a reason for hiding this comment

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

This distinction was created for TypeScript, is it true for GraphQL too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, consider the following example:

query Query {}

type Query

schema {
  query: Query
}

Here Query is used both as the name of the query, and the type of query in the schema

/// range of the name
range: TextRange,
/// If this is a variable binding, it will be defined in an operation scope
scope_id: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

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

We recently refactored the JS semantic model to own u32, you should consider it

[package]
authors.workspace = true
categories.workspace = true
description = "<DESCRIPTION>"
Copy link
Member

Choose a reason for hiding this comment

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

Update the description

@vohoanglong0107 vohoanglong0107 merged commit 5d06dbf into biomejs:main Jul 20, 2024
12 checks passed
@vohoanglong0107 vohoanglong0107 deleted the feat-init-graphql-semantic branch July 20, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants