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

compiler: should definitions know their own name? #708

Closed
Tracked by #641
SimonSapin opened this issue Oct 19, 2023 · 1 comment · Fixed by #727
Closed
Tracked by #641

compiler: should definitions know their own name? #708

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

Comments

@SimonSapin
Copy link
Contributor

As of 1.0.0-beta.4, the Schema struct contains types: IndexMap<Name, ExtendedType>. Each variant of the ExtendedType enum is a struct like this:

pub struct ObjectType {
    pub description: Option<NodeStr>,
    pub implements_interfaces: IndexSet<ComponentStr>,
    pub directives: Directives,
    pub fields: IndexMap<Name, Component<FieldDefinition>>,
}

Notably, these type definition structs do not contain a name: Name field. The reasoning is that the name is already in the map key, so having it in the struct as well would be redundant. And with a mutable data structure, eliminating redundancy eliminates the possibility of inconsistencies.

However:

  1. This is a bit weird and unexpected
  2. This pattern is not applied consistently. For example FieldDefintion is reused from the AST and does contain a name field, despite being in a name-indexed map when in a high-level Schema representation.

I think we should pick one pattern and apply it everywhere. Either nothing in IndexMap<Name, …> should duplicate its name (and have more dedicated structs in the schema module for FieldDefinition and others, instead of reusing AST ones), or everything should. Either way, this is breaking change that needs to happen before 1.0 leaves beta (or not happen).

@SimonSapin SimonSapin added the apollo-compiler issues/PRs pertaining to semantic analysis & validation label Oct 19, 2023
@SimonSapin
Copy link
Contributor Author

We’d like ExtendedType to implement Display by serializing to GraphQL syntax, like many other Rust types representing GraphQL elements do. This requires ExtendedType known its own name.

So proposed plan:

  • Add pub name: Name to all relevant structs (or Option<Name> for executable::Operation)
  • Add fn name(&self) -> &Name to the ExtendedType enum
  • Document that it’s the user’s responsibility when mutating documents to keep names consistent between struct fields and map keys
  • Provide code example for renaming an entry of such a map. (Remove from map at old name, set name field, insert into map at new name)

SimonSapin added a commit that referenced this issue Nov 7, 2023
Fixes #708

In a few places (but not consistently) a `name` field
was omitted from some structs used as map values
on the basis that it would have been redundant with the map key.
This reverts that decision,
making it the user’s responsibility when mutating documents to keep names consistent.

* Add a `pub name: Name` field to `executable::Fragment` as well as
  `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
  in `schema`.
* Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
* Add a `pub fn name: Option<Name>` field to `executable::Operation`
* Remove `executable::OperationRef<'_>`
  (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
  replacing its uses with `&Node<Operation>`
SimonSapin added a commit that referenced this issue Nov 7, 2023
Fixes #708

In a few places (but not consistently) a `name` field
was omitted from some structs used as map values
on the basis that it would have been redundant with the map key.
This reverts that decision,
making it the user’s responsibility when mutating documents to keep names consistent.

* Add a `pub name: Name` field to `executable::Fragment` as well as
  `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
  in `schema`.
* Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
* Add a `pub name: Option<Name>` field to `executable::Operation`
* Remove `executable::OperationRef<'_>`
  (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
  replacing its uses with `&Node<Operation>`
@SimonSapin SimonSapin self-assigned this Nov 7, 2023
SimonSapin added a commit that referenced this issue Nov 7, 2023
Fixes #708

In a few places (but not consistently) a `name` field
was omitted from some structs used as map values
on the basis that it would have been redundant with the map key.
This reverts that decision,
making it the user’s responsibility when mutating documents to keep names consistent.

* Add a `pub name: Name` field to `executable::Fragment` as well as
  `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
  in `schema`.
* Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
* Add a `pub name: Option<Name>` field to `executable::Operation`
* Remove `executable::OperationRef<'_>`
  (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
  replacing its uses with `&Node<Operation>`
SimonSapin added a commit that referenced this issue Nov 9, 2023
* Make everything know their own name

Fixes #708

In a few places (but not consistently) a `name` field
was omitted from some structs used as map values
on the basis that it would have been redundant with the map key.
This reverts that decision,
making it the user’s responsibility when mutating documents to keep names consistent.

* Add a `pub name: Name` field to `executable::Fragment` as well as
  `ScalarType`, `ObjectType`, `InterfaceType`, `EnumType`, `UnionType`, and `InputObjectType`
  in `schema`.
* Add a `fn name(&self) -> &Name` method to the `schema::ExtendedType` enum
* Add a `pub name: Option<Name>` field to `executable::Operation`
* Remove `executable::OperationRef<'_>`
  (which was equivalent to `(Option<&Name>, &Node<Operation>)`),
  replacing its uses with `&Node<Operation>`

* clippy
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.

1 participant