Skip to content

Fix #13185: Type-level module alias for functor parameter accepted#13192

Merged
Octachron merged 3 commits intoocaml:trunkfrom
COCTI:fix13185
May 24, 2024
Merged

Fix #13185: Type-level module alias for functor parameter accepted#13192
Octachron merged 3 commits intoocaml:trunkfrom
COCTI:fix13185

Conversation

@garrigue
Copy link
Copy Markdown
Contributor

Add a new error message when a functor parameter is aliased inside a signature.
Fixes #13185.

(Style.as_inline_code path) p
| Cannot_alias p ->
Location.errorf ~loc
"Cannot alias the module %a, it is a functor argument"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would propose to merge the two clauses into The functor argument %a cannot be aliased.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Functor arguments like %a cannot be aliased?

Changes Outdated
(Jan Midtgaard, review by Antonin Décimo, Sébastien Hinderer, and
David Allsopp)

- #13185: Type-level module alias for functor parameter accepted
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes entry might mislead readers that missed the fact that we are describing the bug and not the fix.
Maybe reject type-level module aliases on functor parameters?

| Mty_alias p ->
if Env.is_functor_arg p env then
raise (Error (pmd.pmd_loc, env, Cannot_alias p));
Mp_absent
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have been wondering if we want to the check this property in transl_modtype when creating the alias, but I think that raising the error here gives a better location : the whole module M = Aliased binding rather than just Aliased.

Copy link
Copy Markdown
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

I agree that it is better to raise an error early for those impossible signatures, and the small implementation size is nice.

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.

Type-level module alias for functor parameter accepted

2 participants