Skip to content

Question mark sn #3

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

Open
wants to merge 5 commits into
base: semantic-non-null
Choose a base branch
from
Open

Conversation

twof
Copy link

@twof twof commented Feb 9, 2025

!!! IMPORTANT !!!

Please Read https://github.com/facebook/graphql/blob/master/CONTRIBUTING.md before creating a Pull Request.

@twof twof changed the title Question mark sn [WIP] Question mark sn Feb 9, 2025
@twof twof changed the title [WIP] Question mark sn Question mark sn Feb 10, 2025
@twof twof marked this pull request as ready for review February 10, 2025 17:03
Copy link
Owner

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Some initial comments.

@@ -1257,6 +1298,17 @@ Type : Name
- {type} must not be {null}
- Return {type}
Copy link
Owner

Choose a reason for hiding this comment

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

I think you want to delete this semantic block

Comment on lines +1301 to +1304
Type : Type ?

- Let {nullableType} be the result of evaluating {Type}
- Return {nullableType}
Copy link
Owner

Choose a reason for hiding this comment

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

This is really confusing; I think delete the Type : Type block above, and instead have:

Suggested change
Type : Type ?
- Let {nullableType} be the result of evaluating {Type}
- Return {nullableType}
Type : Name ?
- Let {name} be the string value of {Name}
- Let {type} be the type defined in the Schema named {name}
- {type} must not be {null}
- Return {type}
Type : ListType ?
- Let {itemType} be the result of evaluating {ListType}
- Let {type} be a List type where {itemType} is the contained type.
- Return {type}

Comment on lines +1908 to +1911
exception that {null} will result in a field error being raised. Semantic-Non-Null
types are only valid in a document that also contains a `@SemanticNullability`
document-level directive. In such a document, Semantic-Non-Null fields have no
adornment, much like nullable fields in documents without that directive.
Copy link
Owner

Choose a reason for hiding this comment

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

The type system should be valid without reference to documents; support for SDL is optional.

Comment on lines +1914 to +1915
In docuements with a `@SemanticNullability` directive, input types are unaltered in
terms of syntax. Unadorned types still represent nullable input types.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
In docuements with a `@SemanticNullability` directive, input types are unaltered in
terms of syntax. Unadorned types still represent nullable input types.
In documents with a `@SemanticNullability` directive, input types are unaltered in
terms of syntax. Unadorned types still represent nullable input types.

Copy link
Owner

Choose a reason for hiding this comment

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

Wait so unadorned is nullable on input but non-null on output? What about if we add something like struct in future that allows the same composite type to be represented on both input and output - you're saying that the input field is allowed to be null, but when it's presented through output it would cause a non-null error to be raised?

@semanticNullability

struct Struct {
  nullable: Int?
  semanticNonNull: Int
  strictNonNull: Int!
}
type Query {
  identity(struct: Struct): Struct
}

query {
  # Valid input since unadorned is nullable on input?
  identity(struct: { nullable: null, semanticNonNull: null, strictNonNull: 0 }) {
    nullable
    # Identity produces error here?
    semanticNonNull
    strictNonNull
  }
}

@@ -2016,6 +2015,7 @@ TypeSystemDirectiveLocation : one of
- `ENUM_VALUE`
- `INPUT_OBJECT`
- `INPUT_FIELD_DEFINITION`
- `DOCUMENT`

Choose a reason for hiding this comment

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

Should we extract document level directives to a separate spec PR, it seems to be lumped in here but sounds like a meaningful addition in the grander scheme either way

Copy link
Author

Choose a reason for hiding this comment

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

Yeah truthfully we could also do no null propagation mode as it's own PR as well.

Copy link
Owner

Choose a reason for hiding this comment

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

These should indeed be separate PRs 👍

@benjie benjie force-pushed the semantic-non-null branch from a77383a to 90c668c Compare March 10, 2025 11:33
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.

3 participants