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: namespace directive #5285

Merged
merged 4 commits into from
Aug 18, 2024
Merged

feat: namespace directive #5285

merged 4 commits into from
Aug 18, 2024

Conversation

mitchellwrosen
Copy link
Member

@mitchellwrosen mitchellwrosen commented Aug 16, 2024

Overview

This PR implements the "namespace directive" feature: a Unison file can contain an optional namespace foo line that affects parsing as follows:

  1. All bound names (type names, constructor names, generated accessor names, term names, and watch expression names) are prefixed with the namespace.
  2. Variables that refer to locally-bound names exactly are prefixed with the namespace.

That is,

namespace a.b.c

type Foo = Bar
type Qux = { quaf : Foo }

baz = ...

honk = ... baz ...

is equivalent to

type a.b.c.Foo = Bar
type a.b.c.Qux = { quaf : a.b.c.Foo }

a.b.c.baz = ...

a.b.c.honk = ... a.b.c.baz ...

Note that the existence of a namespace directive therefore prevents one from referring to a name outside of the file whose suffix matches something in the file.

For example, without a namespace directive, I could write

foo.factorial = ... foo.factorial ... factorial ...

and refer to both my locally-bound foo.factorial as well as a term literally named factorial (no other prefix) in the namespace. (N.B that's not true in trunk today, but it will be fixed soon: #5268).

However, with a namespace directive...

namespace foo

factorial = ???

there's no way to write the same function (because factorial resolves to the locally-bound factorial, which will be expanded to foo.factorial during parsing).

For this reason, it would be unsafe to implement edit.namespace foo naively by putting a namespace foo directive at the top of the block.

Test coverage

I've added a transcript to demonstrate the feature.

@mitchellwrosen mitchellwrosen marked this pull request as ready for review August 16, 2024 19:35
Copy link
Member

@pchiusano pchiusano left a comment

Choose a reason for hiding this comment

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

Awesome!! That was fast.

I'm not sure why the transcript error message regression is happening. Though it might not matter too much if the only behavior is a worse error message in the event you call a definition namespace. Still, would be nice to keep the good message.

@hojberg
Copy link
Member

hojberg commented Aug 16, 2024

Very cool! Can you have multiple namespace directives in a file?

@mitchellwrosen
Copy link
Member Author

@hojberg You cannot – zero or one

@mitchellwrosen
Copy link
Member Author

@pchiusano I looked into this briefly, it really isn't more complicated than we're now parsing namespace as a Reserved lexeme rather than an Identifier lexeme, and go down different error paths from there. This could/should be cleaned up for sure, but I'm inclined to leave it alone for now as it also affects other reserved keywords in the same way.

@pchiusano
Copy link
Member

@mitchellwrosen cool. Yeah I wouldn’t sweat it for this PR then.

@aryairani aryairani merged commit 1c5a4e6 into trunk Aug 18, 2024
35 checks passed
@aryairani aryairani deleted the 24-08-15-namespace-directive branch August 18, 2024 16:30
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.

4 participants