Allow exports from namespaces #1453
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Change in memory usage detected by benchmark. Memory Report for 2c21d22
|
Benchmark for 2c21d22Click to view benchmark
|
|
Change in memory usage detected by benchmark. Memory Report for ef02ab3
|
Benchmark for ef02ab3Click to view benchmark
|
|
Change in memory usage detected by benchmark. Memory Report for 0e74389
|
Benchmark for 0e74389Click to view benchmark
|
|
Change in memory usage detected by benchmark. Memory Report for de39c3f
|
Benchmark for de39c3fClick to view benchmark
|
|
Change in memory usage detected by benchmark. Memory Report for a400509
|
sezna
force-pushed
the
alex/export-statements
branch
from
ec590bb
to
0415cf3
Compare
|
Change in memory usage detected by benchmark. Memory Report for 18711a2
|
Benchmark for 18711a2Click to view benchmark
|
|
Change in memory usage detected by benchmark. Memory Report for 51c39ce
|
Benchmark for 51c39ceClick to view benchmark
|
minestarks
reviewed
May 24, 2024
There was a problem hiding this comment.
Doesn't have to be this PR, but just so I don't forget, we should make a note to update the syntax highlighting grammars (codemirror and TextMate) with the new keywords.
minestarks
reviewed
May 24, 2024
Comment on lines
+521
to
+522
| // bind all exported symbols in a follow-on step | ||
| resolver.resolve_exports(package); |
There was a problem hiding this comment.
I think the behavior is ok here, but the method name and comment confused me.
Would "resolve and bind all imports and exports" be a more complete description of what's happening here?
minestarks
reviewed
May 24, 2024
| #[diagnostic(code("Qsc.Resolve.ExportedNonItem"))] | ||
| ExportedNonItem(#[label] Span), | ||
|
|
||
| #[error("exporting external items is not yet supported")] |
There was a problem hiding this comment.
I'm going to continue my crusade against the word "yet" in errors...
Suggested change
| #[error("exporting external items is not yet supported")] | |
| #[error("exporting external items is not supported")] |
minestarks
reviewed
May 24, 2024
Comment on lines
+125
to
+127
| #[error("export statements are only allowed in a namespace scope")] | ||
| #[diagnostic(code("Qsc.Resolve.ExportFromNonNamespaceScope"))] | ||
| ExportFromNonNamespaceScope(Span), |
There was a problem hiding this comment.
We may want to tweak the error message here, for the user's sake. The word "namespace" won't make sense to them as we don't want them using the namespace keyword anymore, and they won't know what "implicit namespaces" are.
Maybe just state what we don't want to happen, like:
Suggested change
| #[error("export statements are only allowed in a namespace scope")] | |
| #[diagnostic(code("Qsc.Resolve.ExportFromNonNamespaceScope"))] | |
| ExportFromNonNamespaceScope(Span), | |
| #[error("export statements are not allowed in a local scope")] | |
| #[diagnostic(code("Qsc.Resolve.ExportFromLocalScope"))] | |
| ExportFromLocalScope(Span), |
or
Suggested change
| #[error("export statements are only allowed in a namespace scope")] | |
| #[diagnostic(code("Qsc.Resolve.ExportFromNonNamespaceScope"))] | |
| ExportFromNonNamespaceScope(Span), | |
| #[error("export statements are not allowed in a callable body")] | |
| #[diagnostic(code("Qsc.Resolve.ExportFromCallableBody"))] | |
| ExportFromCallableBody(Span), |
or maybe something better if you can think of it.
minestarks
reviewed
May 24, 2024
| @@ -262,6 +272,8 @@ pub enum ItemKind { | |||
| Open(Idents, Option<Box<Ident>>), | |||
| /// A `newtype` declaration. | |||
| Ty(Box<Ident>, Box<TyDef>), | |||
| /// An export declaration | |||
| Export(ExportDecl), | |||
There was a problem hiding this comment.
How do we handle common item properties, like doc comments, attributes and visibility modifiers, on export and import items?
/// doc comment!
@Config(Base)
internal import Bar.BarOperation;I guess the current answer is that they don't cause any errors and have no effect, but is this desirable?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces exports -- a namespace can export any symbol it has access to. External packages (consumers of this package) will only have access to explicitly exported items.
Next up will be imports, which will allow for importing of specific items.
How it works
Exports are a type of
Item, and they appear inside of namespaces (implicit or explicit). Exports are paths, which are resolved to local item ids in the resolver. They are then bound to the scope of that namespace. After that, the exported item is added to the api surface of the namespace.When we support external package dependencies, we only want to allow importing of explicitly exported items. At that time, we will add a flag to the items to denote if they originate from an export.
This PR also supports export aliases:
export { Foo as Bar };