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

Make ast::Name a type wrapper that checks validity #710

Closed
Tracked by #641
SimonSapin opened this issue Oct 19, 2023 · 0 comments · Fixed by #713
Closed
Tracked by #641

Make ast::Name a type wrapper that checks validity #710

SimonSapin opened this issue Oct 19, 2023 · 0 comments · Fixed by #713
Assignees
Labels
apollo-compiler issues/PRs pertaining to semantic analysis & validation

Comments

@SimonSapin
Copy link
Contributor

This is breaking change that we should either make before 1.0 leaves beta, or decide not to make.


In 1.0.0-beta.4 apollo_compiler::ast::Name is a type alias for apollo_compiler::NodeStr, a smart string type with infallible conversion from arbitrary &str. As a result it is possible to generate invalid syntax:

let mut doc = apollo_compiler::ast::Document::parse("{ field }", "input.graphql");
if let apollo_compiler::ast::Definition::OperationDefinition(op) = &mut doc.definitions[0] {
    op.make_mut().name = Some("++".into())
}
assert_eq!(
    doc.serialize().no_indent().to_string(),
    "query ++ { field }"
)

This simple example uses AST, but even full validation in Schema or ExecutableDocument would not catch this.

We could change validation to find every Name and check its validity (must be made of one NameStart character followed by zero or more NameContinue characters) but it would be easy for validation code to miss some, and delay finding a problem until the user actually calls validation.

Proposal

Instead, ast::Name could be changed to a wrapper type of NodeStr that checks validity. Conversion from &str would be fallible. To avoid run-time unwrap when converting from a literal string, there could be a name!("example") macro that checks validity at compile-time.

@SimonSapin SimonSapin added the apollo-compiler issues/PRs pertaining to semantic analysis & validation label Oct 19, 2023
@SimonSapin SimonSapin self-assigned this Oct 20, 2023
@hwillson hwillson added this to the QP Preview (Release 1) milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apollo-compiler issues/PRs pertaining to semantic analysis & validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants