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

gpui: Add Global marker trait #7095

Merged
merged 9 commits into from
Jan 30, 2024
Merged

gpui: Add Global marker trait #7095

merged 9 commits into from
Jan 30, 2024

Conversation

osiewicz
Copy link
Contributor

This should prevent a class of bugs where one queries the wrong type of global, which results in oddities at runtime.

Release Notes:

  • N/A

This should prevent a class of bugs where one queries the wrong type of global, which results in oddities at runtime.

Co-authored-by: Marshall <marshall@zed.dev>
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 30, 2024
@ConradIrwin
Copy link
Member

I'm curious why the existing type stuff didn't work?

@osiewicz
Copy link
Contributor Author

osiewicz commented Jan 30, 2024

It is error prone when you're e.g. refactoring globals (or querying globals in general) - if you use wrong type as the argument of cx.global, you're only gonna notice that at runtime.
This whole thing started in #7023

@maxdeviant
Copy link
Member

It is error prone when you're e.g. refactoring globals (or querying globals in general) - if you use wrong type as the argument of cx.global, you're only gonna notice that at runtime. This whole thing started in #7023

Yea, what the Global trait gives us is finer-grained control over what can be a global (and accessed via the global methods on the context).

So a common pattern we've started doing is having private types implement Global and then expose getters/setters on the inner type. This forces callers to go through a predefined interface instead of having carte blanche access to globals from anywhere.

It should make dealing with globals a lot more manageable (and also more type-safe).

@osiewicz
Copy link
Contributor Author

It is also easier to tell what is global in the codebase.

Call us to get a quote on better type safety. :P

@maxdeviant maxdeviant merged commit e6ebe79 into main Jan 30, 2024
6 checks passed
@maxdeviant maxdeviant deleted the global-marker-trait branch January 30, 2024 19:08
@ConradIrwin
Copy link
Member

Makes sense! I was imagining "oddities" to include type confusion and other odd things, but this makes sense to me

@maxdeviant
Copy link
Member

Makes sense! I was imagining "oddities" to include type confusion and other odd things, but this makes sense to me

Ah yea, the "oddities" is probably better read as "panics" 🤣

maxdeviant added a commit that referenced this pull request Feb 25, 2024
This PR introduces a new `command_palette_hooks` crate that contains the
types used to hook into the behavior of the command palette.

The `CommandPaletteFilter` was previously extracted to the `copilot`
crate in #7095, solely because that was the earliest ancestor of the
crates that depended on it.

The `CommandPaletteInterceptor` was still defined in `command_palette`
itself.

Both of these types were consumed by other crates wanting to influence
the behavior of the command palette, but required taking a dependency on
the entire `command_palette` crate in order to gain access to these
hooks.

By moving them out into their own crate, we can improve the compile
order and make crates like `vim` able to begin building sooner without
having to wait for `command_palette` to finish compiling.

Here's a comparison of the compilation graph before and after (ignore
the timings):

#### Before

<img width="332" alt="Screenshot 2024-02-25 at 12 42 29 PM"
src="https://github.com/zed-industries/zed/assets/1486634/a57c662e-fbc2-41ab-9e30-cca17afa6c73">

#### After

<img width="362" alt="Screenshot 2024-02-25 at 12 51 15 PM"
src="https://github.com/zed-industries/zed/assets/1486634/c1a6d29c-b607-4604-8f1b-e5d318bf8849">

Release Notes:

- N/A
maxdeviant added a commit that referenced this pull request Apr 29, 2024
This PR restores the `Global` trait's status as a marker trait.

This was the original intent from #7095, when it was added, that had
been lost in #9777.

The purpose of the `Global` trait is to statically convey what types can
and can't be accessed as `Global` state, as well as provide a way of
restricting access to said globals.

For example, in the case of the `ThemeRegistry` we have a private
`GlobalThemeRegistry` that is marked as `Global`:
https://github.com/zed-industries/zed/blob/91b3c24ed35d58438ae33970f07d1ff01d3acfc7/crates/theme/src/registry.rs#L25-L34

We're then able to permit reading the `ThemeRegistry` from the
`GlobalThemeRegistry` via a custom getter, while still restricting which
callers are able to mutate the global:
https://github.com/zed-industries/zed/blob/91b3c24ed35d58438ae33970f07d1ff01d3acfc7/crates/theme/src/registry.rs#L46-L61

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants