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

feat(compiler)!: Explicit abstract types #1680

Merged
merged 1 commit into from
Feb 18, 2023
Merged

Conversation

ospencer
Copy link
Member

Previously, any type defined in a module that was not provided was still provided by the compiler as an abstract type. Now, types must explicitly be marked with a new abstract keyword to indicate that the type should be visible but the implementation hidden.

This is a breaking change because types not marked with provide or abstract no longer appear in module signatures. If a value is provided by a module but uses a type that is not also provided or abstract, the compiler shows an error, as this type is private and not accessible by consuming modules.

Closes #1240

@ospencer
Copy link
Member Author

ospencer commented Feb 18, 2023

The errors for non-provided types appearing in provided modules could be expanded to display the specific offending item, but this is a bit complicated and IMO is not necessary for the initial feature. If this PR is approved, I'll create an issue to expand those errors.

@@ -2189,7 +2189,7 @@ let nondep_instance = (env, level, id, ty) => {
list (nl2, tl2). raise Not_found if impossible */
let complete_type_list = (~allow_absent=false, env, nl1, lv2, mty2, nl2, tl2) => {
let id2 = Ident.create("Pkg");
let env' = Env.add_module(id2, mty2, None, env);
let env' = Env.add_module(id2, mty2, None, Location.dummy_loc, env);
Copy link
Member Author

Choose a reason for hiding this comment

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

@phated The dummy_locs are all for places where location information isn't available.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Only a couple questions then I think this is good to go!

@@ -2189,7 +2189,7 @@ let nondep_instance = (env, level, id, ty) => {
list (nl2, tl2). raise Not_found if impossible */
let complete_type_list = (~allow_absent=false, env, nl1, lv2, mty2, nl2, tl2) => {
let id2 = Ident.create("Pkg");
let env' = Env.add_module(id2, mty2, None, env);
let env' = Env.add_module(id2, mty2, None, Location.dummy_loc, env);
Copy link
Member

Choose a reason for hiding this comment

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

Why a dummy_loc here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed getting my comment in soon enough! Just because there's no location information available (this is mostly for module types, a bit of a holdover from OCaml).

@@ -150,5 +150,5 @@ exceptions › exception_4
(call $_gmain)
)
)
;; custom section \"cmi\", size 1238
;; custom section \"cmi\", size 1284
Copy link
Member

Choose a reason for hiding this comment

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

Why would these increase in size instead of decrease?

Copy link
Member Author

Choose a reason for hiding this comment

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

We include location information in CMIs now (we previously stripped all location information), so sizes went up. This doesn't affect final binary sizes and will also help with LSP features, like go to definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I did this because the check just literates the signature, and I essentially am doing a "go to definition" to report the error wherever the error is.)

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

LGTM! Not sure if you want to give anyone else a chance to bikeshed on abstract. I know we asked for alternatives on the community call a few weeks ago and I haven't seen anything.

@ospencer
Copy link
Member Author

Not really... I think we gave this a fair amount of bikeshedding, but if we truly find something we like better we can always send it before 0.6 lands.

@ospencer ospencer added this pull request to the merge queue Feb 18, 2023
Merged via the queue into main with commit 58cd224 Feb 18, 2023
@ospencer ospencer deleted the oscar/explicit-abstract branch February 18, 2023 23:03
av8ta pushed a commit to av8ta/grain that referenced this pull request Apr 11, 2023
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.

Compiler: Don't expose unnecessary types in module signatures
2 participants