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

Implement private modules #7840

Merged
merged 37 commits into from
Oct 4, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Sep 19, 2023

Closes #7304

Pull Request Description

Adds the ability to declare a module as private. Modifies the parser to add the private keyword as a reserved keyword. All the checks for private modules are implemented as an independent Compiler pass. No checks are done at runtime.

Important Notes

  • Introduces new keyword - private - a reserved keyword.
  • Modules that have private keyword as the first statement are declared as private (Project private)
  • Public module cannot have private submodules and vice versa.
    • This would require runtime access checks
  • See Encapsulation of project with private keyword #7088 for the specification.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Akirathan Akirathan self-assigned this Sep 19, 2023
@Akirathan Akirathan force-pushed the wip/akirathan/7304-Implement-private-modules branch from 3b2e5e5 to 08513fe Compare September 19, 2023 11:49
@@ -951,6 +955,7 @@ pub fn apply_unary_operator<'s>(opr: token::Operator<'s>, rhs: Option<Tree<'s>>)
pub fn to_ast(token: Token) -> Tree {
match token.variant {
token::Variant::Ident(ident) => token.with_variant(ident).into(),
token::Variant::Private(private) => Tree::private(token.with_variant(private)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will never match because the Private token type is only generated by the macro, which immediately places it in a AST node, so an Item::Token(Token::Private) is never encountered.

I don't see a need for a token type for private; if it were a reserved word we could easily identify at lex time and always treat specially then we should give it a token type, but we are taking a contextual keywords approach (because it is a goal that identifiers with special meanings in syntactic constructs can be used as normal identifiers when used outside those constructs).

println!("private_keyword[begin] : Segments={:?}", segments);
let segment = segments.pop().0;
let keyword = into_private(segment.header);
let ret_tree = syntax::Tree::private(keyword);
Copy link
Contributor

Choose a reason for hiding this comment

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

The body can be handled the same way as in capture_expressions, and placed in an Option<Tree> field of the private node. (I think an optional body is preferable to separate Tree types for the standalone-private and private-with-expression because we have to assume the user could be halfway through typing something, so a Private without a body may well be private-with-expression whose expression hasn't been written yet).


#[test]
#[ignore]
fn private_is_first_statement() {
Copy link
Contributor

@kazcw kazcw Sep 19, 2023

Choose a reason for hiding this comment

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

This will require changes to the macro resolver--we could add a field to Context::Statement, like so:
enum Context { Expression, Statement { first: bool } }
The resolver would then keep track of when the first "contentful' line has been encountered in order to construct the first field. Then we could add a third MacroMap that is only run in Statement context with first=true. Note that there's a rewrite of the macro resolver currently in CI: #7711.

#[rustfmt::skip]
let lines = vec![
"# Some comment",
"# Other comment",
Copy link
Contributor

@kazcw kazcw Sep 19, 2023

Choose a reason for hiding this comment

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

Important also to allow doc-comments. That may be tricky, actually. I'm thinking about how that would work...

Copy link
Contributor

Choose a reason for hiding this comment

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

We can just check for Documented nodes and treat them as non-contentful. They are bodiless during macro resolution because that type of multi-line construct is assembled after macros are run.

docs/semantics/encapsulation.md Outdated Show resolved Hide resolved
docs/semantics/encapsulation.md Outdated Show resolved Hide resolved
docs/semantics/encapsulation.md Outdated Show resolved Hide resolved
Akirathan and others added 3 commits October 2, 2023 16:48
Co-authored-by: Jaroslav Tulach <jaroslav.tulach@enso.org>
….com:enso-org/enso into wip/akirathan/7304-Implement-private-modules
@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Oct 3, 2023
@Akirathan Akirathan added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Oct 3, 2023
@Akirathan Akirathan removed the CI: Keep up to date Automatically update this PR to the latest develop. label Oct 4, 2023
Comment on lines +160 to +166
if (subModName.getParent().isDefined()) {
return parentModName.item().equals(
subModName.getParent().get().item()
);
} else {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

getOrElse?

} else {
throw new IllegalStateException("unknown exp: " + exp);
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? Shouldn't this be return false;?

@mergify mergify bot merged commit c22928e into develop Oct 4, 2023
35 checks passed
@mergify mergify bot deleted the wip/akirathan/7304-Implement-private-modules branch October 4, 2023 10:33
@Akirathan Akirathan mentioned this pull request Nov 1, 2023
5 tasks
@Akirathan Akirathan mentioned this pull request Dec 18, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement private modules
5 participants