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

Private foreign function should result in parser error #10137

Closed
1 task
Akirathan opened this issue May 30, 2024 · 1 comment · Fixed by #10734
Closed
1 task

Private foreign function should result in parser error #10137

Akirathan opened this issue May 30, 2024 · 1 comment · Fixed by #10734
Assignees
Labels

Comments

@Akirathan
Copy link
Member

Akirathan commented May 30, 2024

Follow-up of #10060

The following snippet:

private foreign js js_func = """
    return 42;

Should result in parsing error. Currently, the parser returns:

(Private (Function (Ident foreign) #((() (Ident js) () ()) (() (Ident js_func) () ())) () "=" (TextLiteral #((Section "return 42;")))))

but it should result in an invalid tree. Or even better in (Private (ForeignFunction ...)).

Failed attempts

diff --git a/lib/rust/parser/debug/tests/parse.rs b/lib/rust/parser/debug/tests/parse.rs
index b9d7e0489..c1e0c3ce6 100644
--- a/lib/rust/parser/debug/tests/parse.rs
+++ b/lib/rust/parser/debug/tests/parse.rs
@@ -492,6 +492,12 @@ fn foreign_functions() {
             #((() (Ident a) () ()) (() (Ident b) () ()))
             "="
             (TextLiteral #((Section "42")))));
+
+    test!("private foreign python my_method a b = \"42\"",
+        (Private (ForeignFunction foreign python my_method
+            #((() (Ident a) () ()) (() (Ident b) () ()))
+            "="
+            (TextLiteral #((Section "42"))))));
 }
 
 #[test]
diff --git a/lib/rust/parser/src/macros/built_in.rs b/lib/rust/parser/src/macros/built_in.rs
index f06804ac6..cd1fdefd7 100644
--- a/lib/rust/parser/src/macros/built_in.rs
+++ b/lib/rust/parser/src/macros/built_in.rs
@@ -753,9 +753,29 @@ fn private_keyword<'s>(
     precedence: &mut operator::Precedence<'s>,
 ) -> syntax::Tree<'s> {
     use syntax::tree::*;
-    let segment = segments.pop().0;
-    let keyword = into_private(segment.header);
-    let body_opt = precedence.resolve(segment.result.tokens());
+    let segment = segments.pop();
+    let matched_segment = segment.0;
+    let rest_of_segment = segment.1;
+    println!("private_keyword: matched_segment={:#?}", matched_segment);
+    println!("private_keyword: rest_of_segment={:#?}", rest_of_segment);
+    let cloned_result = matched_segment.result.clone();
+    let cloned_tokens = cloned_result.tokens();
+    let first_item = cloned_tokens.first().unwrap().clone();
+    println!("private_keyword: first_item={:#?}", first_item);
+    let first_ident = try_into_token(first_item)
+    .and_then(try_token_into_ident);
+    if let Some(mut id) = first_ident && id.code == "foreign" {
+        println!("private_keyword: first token in body is 'foreign' identifier");
+        id.left_offset += matched_segment.header.left_offset;
+        // TODO: Add left offset of the private keyword?
+        let foreign_body_opt = try_foreign_body(id, cloned_tokens, precedence);
+        if foreign_body_opt.is_ok() {
+            println!("private_keyword: body is a foreign function");
+            return foreign_body_opt.unwrap();
+        }
+    }
+    let keyword = into_private(matched_segment.header);
+    let body_opt = precedence.resolve(matched_segment.result.tokens());
     match body_opt {
         Some(body) => {
             let statement = crate::expression_to_statement(body);

Tasks

Preview Give feedback
@kazcw
Copy link
Contributor

kazcw commented Jun 3, 2024

Currently we treat the body of any macro as expression context. For private, we need a new behavior in the macro resolver: When handling Step::StartSegment(header), leave the context unchanged so that if private is in statement context, we recognize functions etc in its body.
One way to do this would be to add a new field to macro::Definition to mark whether it has this context-preserving property, and in the resolver include that information in Step::StartSegment (maybe as an Option<Context>, that is either None: preserve or Some(Context::Expression): set to Expression).

@Akirathan Akirathan added the p-low Low priority label Jun 3, 2024
kazcw added a commit that referenced this issue Aug 1, 2024
Implements #5453.

Syntax changes:
- Eliminate old non-decimal number syntax.
- Unspaced application like `f(x)` is now an error.
- Applying `:` to a non-QN in statement context is allowed and produces
  a `TypeAnnotated` (fixes #6152).

API changes:
- All fixed-content tokens are now distinct types.

Error improvements (fixes #5444), especially for:
- Out-of-context expressions/statements
- Statement syntaxes
- Named-app syntax
- Unspaced-application errors
- Number syntax
- Private annotations (fixes #10137)
- Parens (fixes #6741)
- Type defs (fixes #8633)
- Fix some panics caused by invalid expressions, found by parsing non-Enso code.
- Reject some operations in pattern context, e.g. `1 + 1 = 2`.
- Eliminate `export` with `all` or `hiding` (#10258).

Improve Rust parsing performance by 33%; now 20 MB/s on my bench machine:
- Stream lexer to parser.
- New, faster parser for type defs, statements, numbers.
- More efficient tree layout (fixes #5452).
Improve backend parsing performance additionally:
- Backend now uses optimized parser build (in debug builds,
  debug-assertions are still enabled).

Build improvements:
- Fix some redundancy between `sbt` and build script: now only `sbt`
  compiles JNI and generates parser's Java bindings for backend use.
  Build script generates Java to a different directory when parser
  serialization self-test is requested.
- Improve `sbt` detection of changed parser; this should reduce the need
  for clean builds in CI.

Testing:
- Add binary target for fuzzing with AFL.
@kazcw kazcw moved this from ❓New to 👁️ Code review in Issues Board Aug 1, 2024
@mergify mergify bot closed this as completed in #10734 Aug 5, 2024
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Aug 5, 2024
@farmaazon farmaazon moved this from 🟢 Accepted to 🗄️ Archived in Issues Board Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants