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

✨ Introduce an @internal attribute for individual declarations. #2486

Closed
hayleigh-dot-dev opened this issue Dec 23, 2023 · 23 comments
Closed
Labels
help wanted Contributions encouraged priority:medium

Comments

@hayleigh-dot-dev
Copy link
Member

Background

Currently we have the internal_modules entry in our gleam.toml as a way of excluding certain modules from the generated documentation. Any pub code is always importable, but by excluding it from the docs it is de facto internal and not part of the public API.

This solution works well when you have large chunks of internal functionality, but quickly gets clunky when you have small bits of internal functionality next to public code.

It is good practice to define custom types as pub opaque unless there is good reason to do otherwise: this means consumers can't pattern match on your custom types and the implementation details remain hidden. Because of this though, it is not possible to have a pub opaque type part of the public API and a separate internal module that can pattern match on that type.

Propsal

I'm proposing an @internal attribute that can be attached to declarations that hides that declaration from the generated documentation in an otherwise-public module:

pub opaque type Wibble {
  Wibble
  Wobble
}

@internal 
pub fn encode(wibbble: Wibble) -> Json {
  case wibble {
    Wibble -> ...
    Wobble -> ...
  }
} 

Here this encode function needs access to the structure of the Wibble type in order to encode it to JSON properly, but the function itself is not part of the public API. This is handy if we have other parts of a package that might need to know how to encode a Wibble.


Prior art

Elixir has this functionality by setting @doc false, see here

@lpil
Copy link
Member

lpil commented Dec 23, 2023

Seems reasonable to me. We will see if anyone has any thoughts or adjustments or objections.

It should also be excluded from autocompletions from other packages.

@hayleigh-dot-dev hayleigh-dot-dev changed the title ✨ Introduce an @internal ✨ Introduce an @internal attribute for individual declarations. Dec 27, 2023
@massivefermion
Copy link
Contributor

Makes sense to me. Gonna keep my eye on this issue and start working on it when it's final.

@erikareads
Copy link
Contributor

I could use this too, I like the current @internal proposal.

@lpil lpil added help wanted Contributions encouraged priority:medium labels Jan 9, 2024
@cauebs
Copy link

cauebs commented Jan 9, 2024

Won't this be a little bit confusing? First, because it seems somewhat symmetric to @external while having nothing to do with it. Second, because then you can have something marked as pub that isn't exactly public? Users would then be able to use items that are not documented. Are breaking changes to these items allowed without affecting semver?

Looking at Rust for alternative ideas, there are two things that I understand would satisfy the current proposal's goals: there's the #[doc(hidden)] attribute, and there's the pub(crate) visibility modifier, both reasonably self-explanatory.

Couldn't we go one of these ways?

@massivefermion
Copy link
Contributor

@cauebs brings up a very good point. It can't be @internal. Maybe @hidden? or @no_doc? something like that!

@erikareads
Copy link
Contributor

We already have a notion of internal that works this way:

# Modules that should be considered "internal" and will not be included in
# generated documentation. Note this currently only affects documentation;
# public types and functions defined in these modules are still public.
#
# Items in this list are "globs" that are matched against module names. See:
# https://docs.rs/glob/latest/glob/struct.Pattern.html
#
# The default value is as below, with the `name` of your project substituted in
# place of "my_app".
internal_modules = [
  "my_app/internal",
  "my_app/internal/*",
]

Having public functions that aren't visible is useful for FFI and Testing.

Having an @library or something that matches pub(crate) would be a stronger guarantee for what I need, but that will likely require more significant work.

@massivefermion
Copy link
Contributor

I know, but when it's a config in gleam.toml, there is no implicit association with @external but having both @external and @internal when they have nothing to do with each other is confusing!

@lpil
Copy link
Member

lpil commented Jan 9, 2024

Won't this be a little bit confusing? First, because it seems somewhat symmetric to @external while having nothing to do with it.

It hasn't come up yet with internal modules, but I can see what you mean. We could rename them both to something else if we have a better name.

In OOP this is sometimes called protected, but I'm not a fan of that name.

Second, because then you can have something marked as pub that isn't exactly public? Users would then be able to use items that are not documented.

Aye, that's the goal of the feature. It's a statement about the contract between the user and this program.

Are breaking changes to these items allowed without affecting semver?

They would be, aye.

@erikareads
Copy link
Contributor

hidden_modules = [...]
@hidden
pub type HiddenThing

@lpil
Copy link
Member

lpil commented Jan 10, 2024

I'm not a huge fan of hidden as I think that name describes a consequence of what they are, but not what they are. The contract of them being externally private is far more important than them being hidden from documentation.

@massivefermion
Copy link
Contributor

@restricted, @reserved, @exclusive?

@lpil
Copy link
Member

lpil commented Jan 10, 2024

Perhaps @unsupported ?

@massivefermion
Copy link
Contributor

Perhaps @unsupported ?

I don't think it accurately describes what it does. The advantage with my three suggestions is that anyone seeing them for the first time, without any explanation, could arrive at some vague conclusion of what they do!

@lpil
Copy link
Member

lpil commented Jan 10, 2024

You say that, but I don't know what any of those three words mean in this context. They're not restricted or reserved in any way, and I'm not sure what they're exclusive of.

I find @unsupported and @internal more clear as they're functions for which support is not provided if you use them due to their being internal to the package itself.

@hayleigh-dot-dev
Copy link
Member Author

I don't agree that @internal has some confusing conflation with @external. If you take more than a second to consider it, an internal dual to @external makes no logical sense: all gleam things are internal except for externals, you'd immediately realise that such an annotation can't possibly mean anything to do with @external.

@internal describes, precisely, what the annotation does. It marks a declaration as intended for internal use. It is still public and consumable by user code, by design, so any word that implies it is somehow hidden from those people doesn't make sense.

  • @restricted implies it cannot be used.
  • @reserved implies the name is unique.
  • @exclusive implies it cannot be used.

@massivefermion
Copy link
Contributor

my bad! Although I still don't see @unsupported implying what we mean here. Just to make myself clear, from the perspective of the user of the package, using the definition from that package is unsupported, but from the perspective of the author, it doesn't make sense, what is it that is not supported?! I just don't see it.
Sorry, I think I'm starting to ramble again, last words, I'm now more on the side of hayleigh's @internal.

@hayleigh-dot-dev
Copy link
Member Author

Remember that the purpose of this annotation is to hide a declaration from the generated docs. That means anyone who sees this annotation is necessarily looking at the source code! That means they are beyond the realms of a normal package consumer, and are now someone,, looking at the internals!

From that perspective, it becomes immediately valuable to see a pub declaration with an @internal attribute because its telling you - non-author snooping at the source - that this particular function is for internal use and you should use at your own risk.

I think I've convinced myself further that @internal is the right word.


Unrelatedly, to add more support to this issue in general. It would be nice to be able to mark the main function of a module as internal too. I always find it very odd when I'm looking at package docs with a main just hanging out.

@lpil
Copy link
Member

lpil commented Jan 11, 2024

from the perspective of the user of the package, using the definition from that package is unsupported, but from the perspective of the author, it doesn't make sense, what is it that is not supported?! I just don't see it.

This is the same for pub. Those functions are public to the module author, but not that programmers' users.

The programmer wrote this code, they are making statements about it to other people rather than themselves.

@cdaringe
Copy link

we have pub type Foo, but we could also entertain rust type pub (module) type Foo for scoped public-ness.

in this respect, i could put my pub (module) opaque type in internal/*, and because it's a pub (module) ..., it's permitted to be consumed in my module.

i may not be grokking the proposal perfectly, but I prefer something like the rust strategy over @internal. in the example above, encode was decorated with @internal to work around an issue with Wibble. my intuition suggests that fixing the scoping with Wibble can and should be handled at Wibble, rather than by consumers of Wibble.

🤔

@giacomocavalieri
Copy link
Member

I agree with Hayleigh and @internal also feels very natural once someone is also exposed to the idea of internal modules: the annotation basically does the same thing but on a per-definition basis instead of being applied to a whole module.
That's something I felt I could use a couple of times: "it would be really nice to hide this function from the public API but I don't want (or I can't) move this to an internal module", to which a natural answer is "make it an internal definition!"

Also to add to the issue, we would need to warn if any type marked as @internal is inadvertently exposed in the public api (just like suggested here #2234)

@lpil
Copy link
Member

lpil commented Jan 18, 2024

@cdaringe This is similar but not the same as making an item public or private for a package or set of modules.

I would also be interested in a system for drawing lines within a package to enforce certain relationships between subsystems, but we don't have a good design yet. Rust's system is capable but I've never seen anyone do more than pub(crate), which makes me think it's not a good design- it doesn't matter if it's capable of doing something if no one wants to use it.

If we determine a good design for this it can be added in future. Both this internal system and a more powerful public/private system have their own uses.

@giacomocavalieri
Copy link
Member

I think this can be closed now 🥳

@lpil
Copy link
Member

lpil commented Mar 25, 2024

Thank you!!

@lpil lpil closed this as completed Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions encouraged priority:medium
Projects
None yet
Development

No branches or pull requests

7 participants