Atom constructors can be private #9692

Akirathan · 2024-04-12T16:29:22Z

Pull Request Description

Atom constructors can be declared as private (project-private). project-private constructors can be called only from the same project. See the encapsulation.md docs for more info.

Important Notes

Parser supports an optional body for the private keyword. It now accepts an atom constructors as optional body. Private methods are considered a syntactical error for now.
The runtime private access check works on Function and FunctionSchema and AtomConstructor data. No need to fiddle with truffle's frames.
If a private constructor is called from different project, a panic with Standard.Base.Errors.Common.Private_Access payload is thrown.
Foreign polyglot code cannot access anything private. For foreign code, all the private entities are not visible.
Usage of private constructors in pattern matching is checked during compilation.
Due to current compiler limitations, users still can reference the project-private constructor without a compilation issue.
- See Atom constructors can be private #9692 (comment)

Runtime private checks are implemented in such a way, that it should be easy to implement private methods in the future. For private methods, only parser modifications should be necessary.

Checklist

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

The documentation has been updated, if necessary.
All code follows the
Scala,
Java
All code has been tested:
- Unit tests have been written where possible.

It only checks that there is no mix of public and private constructors in types.

kazcw · 2024-04-24T17:13:34Z

lib/rust/parser/src/macros/built_in.rs

+                box syntax::tree::Variant::ConstructorDefinition(_) =>
+                    syntax::Tree::private(keyword, Some(new_body)),
+                _ => syntax::Tree::private(keyword, Some(body))
+                    .with_error("Unsupported private body."),


This is a user-facing error--it should explain why the syntax is invalid to a user with no knowledge of the parser internals

Suggested change

.with_error("Unsupported private body."),

.with_error("Unsupported private entity."),

Is this better? Or do you advice to print the whole body content with something like body.code? Note that body is of type Tree<'{error}> here.

kazcw · 2024-04-24T17:27:41Z

lib/rust/parser/src/macros/built_in.rs

+    match body_opt {
+        Some(body) => {
+            let body_ = body.clone();
+            let new_body = to_body_statement(body_);


I think this test would fail: expect_invalid_node("private ConstructorOutsideType"). Whether to_body_statement is applied must depend on whether the expression is in the top level of a type definition, not the expression's content.

The right solution here is a bit complex because when private_keyword is called, we don't know yet if the private invocation occurred in the context of a type-definition body, so we can't tell yet whether the inner expression should be interpreted as a type body statement (and potentially a constructor definition).

What we could do is:

If the usage of the keyword is not valid in the default (expression) context, wrap it in an Invalid node (i.e. with_error("The 'private' keyword cannot be applied to any expression outside of a type definition.")).

In to_body_statement, recognize (Invalid (Private ...)), discard the outer Invalid, apply the to_body_statement transformation to the body of the private, and then check whether it's valid in context, emitting a new error if it isn't a constructor.

I think this test would fail: expect_invalid_node("private ConstructorOutsideType"). Whether to_body_statement is applied must depend on whether the expression is in the top level of a type definition, not the expression's content.

@kazcw Yes, it fails. I have added that test in 395544f.

Your proposed solution would work for private constructors. But in the near future, we would like to support private methods as well. Not only methods in types, but also static methods in top scope in a module. Any suggestions how to support the private constructor and be able to parse also private top-scope method in the future?

expression_to_statement (where method definitions are recognized) would need a clause analogous to the new logic in to_body_statement.

@kazcw Please help. I am struggling to implement your suggestion in fcf2db5. That commit does not compile - there is a borrow checker error that I cannot resolve. Would you please be so kind and fix that commit?

I refactored it, pushed @ 4a11104

Akirathan · 2024-04-25T15:32:11Z

@JaroslavTulach Even with PrivateSymbolsAnalysis compiler pass, that scans all BindingsMap.Resolution metadata in the IR, introduced in 4ba8b53, we can still reference project-private constructor from different project without any compilation error. As you can see in the following IR graph:

There is no BindingsMap.Resolution metadata for the Cons literal - which is in this case known to be a project-private constructor. This is a limitation of our current approach to fully qualified names, and solving it requires huge effort. I am afraid we will have to merge this PR knowing that for these cases, there will be "just" a runtime error...

# Conflicts: # CHANGELOG.md

radeusgd · 2024-04-29T11:04:00Z

test/Base_Tests/package.yaml

+# Base_Tests import stuff from Helpers sibling project. So we need this property here.
+prefer-local-libraries: true


radeusgd · 2024-04-29T11:06:52Z

test/Base_Tests/src/Semantic/Private_Spec.enso

+        # Note that pattern matching on project-private constructor from a different project
+        # is a compilation error. So we can only test it for the same project.


Nice comment. We could leave a link to a corresponding test in the runtime-integration-tests that tests the compilation error case.

That test is in PrivateAccessTest.java:cannotPatternMatchOnPrivateConstructorFromDifferentProject

radeusgd · 2024-04-29T11:23:31Z

engine/runtime-parser/src/test/java/org/enso/compiler/core/EnsoParserTest.java

+    // Mixing public and private constructor is a semantic error, not a syntax error.
+    // So parsing should be fine.
+    parseTest(
+        """
+            type My_Type
+                private Ctor_1 a b
+                Ctor_2 c d
+            """);


I appreciate the comment explaining the intention here 👍

radeusgd

Checked the .enso files and tests checking the overall semantics - all looks good to me

Akirathan added 2 commits April 12, 2024 18:28

Add some parse tests

a762f17

Fix docs

e0eed43

Akirathan self-assigned this Apr 12, 2024

Parser supports private constructor definitions

9750ef8

Akirathan force-pushed the wip/akirathan/8836-private-ctors branch from 033cd27 to 9750ef8 Compare April 16, 2024 09:29

Akirathan added 24 commits April 16, 2024 17:03

TreeToIr handles private constructor

4b79760

Add some ErrorCompilerTest

957ac75

Add PrivateConstructorAnalysis compiler pass

9c07f84

Add some rust tests

0366e68

fmt

c881214

Add some tests for the runtime access check.

421dd6e

Refactor FunctionSchema ctor to builder pattern.

dd5f590

Add Private_Access builtin error

1228174

Add projectPrivate field to FunctionSchema and IR-related types

04d0f5e

If type has one project-private ctor, all ctors are project-private

ab617f1

AtomConstructor generates Functions with optional projectPrivate field

770de5f

fix typos in docs

4275440

InvokeFunctionNode checks private access

2140e36

fmt

c0266ca

GetFieldNode is EnsoRootNode

09d3e2f

fix test build

eefc514

InvokeFunctionNode also checks private constructor access

25089ee

Merge branch 'refs/heads/develop' into wip/akirathan/8836-private-ctors

c8c0bb1

QualifiedAccessorNode extends EnsoRootNode

1cc1262

fmt

6f64272

Fix PassesTest

aef6e55

EnsoRootNode.moduleScope may be null

ead434d

Update changelog

a53a2d1

Improve error message

85dca02

Akirathan force-pushed the wip/akirathan/8836-private-ctors branch from bb05165 to 85dca02 Compare April 19, 2024 12:10

Akirathan added 4 commits April 23, 2024 18:33

PrivateConstructorAnalysis pass handles also patterns

cbdaf85

Add test for pattern matching on private constructor from same project

c2540b2

Revert PrivateConstructorAnalysis pass.

4f6f8db

It only checks that there is no mix of public and private constructors in types.

PrivateConstructor error is a Resolution error, not Pattern error.

c983c7f

kazcw requested changes Apr 24, 2024

View reviewed changes

Akirathan added 2 commits April 25, 2024 16:48

Add PrivateSymbolAnalysis pass

4ba8b53

Fix project-private type recognition in RuntimeStubsGenerator

6ac8d8a

Akirathan and others added 9 commits April 25, 2024 17:52

Add a failing test on private constructor to parser

395544f

Merge branch 'refs/heads/develop' into wip/akirathan/8836-private-ctors

7c91a0b

# Conflicts: # CHANGELOG.md

[WIP] to_body_statement deals with Invalid(Private(_))

fcf2db5

Fix

4a11104

Merge branch 'refs/heads/develop' into wip/akirathan/8836-private-ctors

fe93ffa

fmt

656124a

InvokeFunctionNode.makePrivateAccessPanic is behind TruffleBoundary

e70dba7

Fix the polyglot contract for private members

d0ab649

Fix improved method name in Auto_Constructor_Instrumentation_Spec.enso

fc0b0db

radeusgd reviewed Apr 29, 2024

View reviewed changes

Akirathan added 2 commits April 29, 2024 13:12

Private_Access error has qualified project names

70ee2b3

Add TruffleBoundary to fix native-image build

85e851b

radeusgd reviewed Apr 29, 2024

View reviewed changes

Run prettier on markdown

c0125a0

radeusgd approved these changes Apr 29, 2024

View reviewed changes

kazcw approved these changes Apr 29, 2024

View reviewed changes

Akirathan merged commit 660c5e7 into develop Apr 29, 2024
36 checks passed

Akirathan deleted the wip/akirathan/8836-private-ctors branch April 29, 2024 12:43

This was referenced Apr 29, 2024

Hide the Table internals in Table and DB_Table #9807

Open

Add Vector.build_multiple and handle dataflow error and warning propagation in build and build_multiple #9766

Merged

Akirathan mentioned this pull request May 24, 2024

Implement private methods #10060

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atom constructors can be private #9692

Atom constructors can be private #9692

Akirathan commented Apr 12, 2024 •

edited

kazcw Apr 24, 2024

Akirathan Apr 25, 2024

kazcw Apr 24, 2024

Akirathan Apr 25, 2024

kazcw Apr 25, 2024

Akirathan Apr 26, 2024

kazcw Apr 29, 2024

Akirathan commented Apr 25, 2024

radeusgd Apr 29, 2024

radeusgd Apr 29, 2024

Akirathan Apr 29, 2024

radeusgd Apr 29, 2024

radeusgd left a comment

	.with_error("Unsupported private body."),
	.with_error("Unsupported private entity."),

		# Base_Tests import stuff from Helpers sibling project. So we need this property here.
		prefer-local-libraries: true

		# Note that pattern matching on project-private constructor from a different project
		# is a compilation error. So we can only test it for the same project.

Atom constructors can be private #9692

Atom constructors can be private #9692

Conversation

Akirathan commented Apr 12, 2024 • edited

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akirathan commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Akirathan commented Apr 12, 2024 •

edited