internal: KCL modules, part 1 #4149

jtran · 2024-10-13T19:03:47Z

Addresses #4080. (Not ready to close it yet.)

Important

Requires a fix for #4147 before it can work in ZMA.

Overview

// numbers.kcl
export fn inc = (x) => {
  return x + 1
}

import inc from "numbers.kcl"

answer = inc(41)

This also implements multiple imports with optional renaming.

import inc, dec from "numbers.kcl"
import identity as id, length as len from "utils.kcl"

Note: Imported files must be in the same directory.

Things for a follow-up PR:

[BUG]: Import does not work for any *.obj *.step #4147. Currently, we cannot read files in WebAssembly, i.e. ZMA.
Docs
Should be an error to import anywhere besides the top level. Needs parser restructuring to track the context of a "function body".
Should be an error to have export anywhere besides the top level. It has no effect, but we should tell people it's not valid instead of silently ignoring it.
Error message for cycle detection is funky because the Rust side doesn't actually know the name of the first file. Message will say "b -> a -> b" instead of "a -> b -> a" when "a" is the top-level file.
Cache imported files so that they don't need to be re-parsed and re-executed.

qa-wolf · 2024-10-13T19:03:50Z

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

vercel · 2024-10-13T19:03:52Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Oct 16, 2024 10:44pm

codecov · 2024-10-13T19:26:42Z

Codecov Report

Attention: Patch coverage is 75.84746% with 114 lines in your changes missing coverage. Please review.

Project coverage is 85.83%. Comparing base (7d44de0) to head (eddbf61).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/wasm-lib/kcl/src/ast/types.rs	22.89%	64 Missing ⚠️
src/wasm-lib/kcl/src/executor.rs	87.06%	15 Missing ⚠️
src/wasm-lib/kcl/src/parser/parser_impl.rs	91.58%	9 Missing ⚠️
src/wasm-lib/kcl-to-core/src/conn_mock_core.rs	45.45%	6 Missing ⚠️
src/wasm-lib/kcl/src/engine/conn.rs	45.45%	6 Missing ⚠️
src/wasm-lib/kcl/src/walk/ast_walk.rs	0.00%	5 Missing ⚠️
src/wasm-lib/kcl/src/fs/local.rs	50.00%	4 Missing ⚠️
src/wasm-lib/kcl/src/errors.rs	60.00%	2 Missing ⚠️
src/wasm-lib/kcl-test-server/src/lib.rs	0.00%	1 Missing ⚠️
src/wasm-lib/kcl/src/ast/modify.rs	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4149      +/-   ##
==========================================
- Coverage   85.99%   85.83%   -0.16%     
==========================================
  Files          76       76              
  Lines       26700    27155     +455     
==========================================
+ Hits        22960    23309     +349     
- Misses       3740     3846     +106

Flag	Coverage Δ
wasm-lib	`85.83% <75.84%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nrc

Looks good to me! I'm pretty unfamiliar with the engine and program memory code, so I'd want to get Adam to take a look too.

src/wasm-lib/kcl/src/ast/types.rs

nrc · 2024-10-14T00:22:05Z

src/wasm-lib/kcl/src/executor.rs

+    /// this is empty, we're not currently executing a use statement.
+    pub import_stack: Vec<std::path::PathBuf>,
+    /// The directory of the current project.  This is used for resolving import
+    /// paths.


Could you document when it can be None and what that means? Depending on the answer, it might be better to use a custom enum rather than Option

I assume 'none' means current working directory but yeah should be documented

I added an enum, so hopefully it's clearer. 282225c

Wait, the ProjectDirectory enum that I made is giving me trouble with wasm. I get the error "the trait FromWasmAbi is not implemented for ProjectDirectory". wasm_bindgen doesn't support them rustwasm/wasm-bindgen#2407.

How are we able to use enums with values elsewhere, like here?

modeling-app/src/wasm-lib/kcl/src/ast/types.rs

Lines 453 to 461 in fbac993

#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, Bake)]

#[databake(path = kcl_lib::ast::types)]

#[ts(export)]

#[serde(tag = "type")]

pub enum BodyItem {

ExpressionStatement(ExpressionStatement),

VariableDeclaration(VariableDeclaration),

ReturnStatement(ReturnStatement),

}

I reverted back to the Option<String> and added a comment explaining None.

src/wasm-lib/kcl/src/unparser.rs

adamchalmers · 2024-10-14T20:42:13Z

src/wasm-lib/kcl-test-server/src/lib.rs

@@ -178,7 +178,7 @@ async fn snapshot_endpoint(body: Bytes, state: ExecutorContext) -> Response<Body
    // Let users know if the test is taking a long time.
    let (done_tx, done_rx) = oneshot::channel::<()>();
    let timer = time_until(done_rx);
-    let snapshot = match state.execute_and_prepare_snapshot(&program, id_generator).await {
+    let snapshot = match state.execute_and_prepare_snapshot(&program, id_generator, None).await {


If you avoid modifying the execute_and_prepare_snapshot method, this PR diff will be much smaller (just make a new method that lets you take the optional param instead, and make the existing one set it to None)

src/wasm-lib/kcl/src/ast/types.rs

adamchalmers · 2024-10-14T20:50:29Z

src/wasm-lib/kcl/src/executor.rs

+    /// this is empty, we're not currently executing a use statement.
+    pub import_stack: Vec<std::path::PathBuf>,
+    /// The directory of the current project.  This is used for resolving import
+    /// paths.


I assume 'none' means current working directory but yeah should be documented

adamchalmers · 2024-10-14T20:54:31Z

src/wasm-lib/kcl/src/executor.rs

+                            source_ranges: vec![import_stmt.into()],
+                        }));
+                    }
+                    let source = self.fs.read_to_string(&resolved_path, source_range).await?;


Doesn't have to be in this PR, but eventually, we'll want to cache the "read, parse, evaluate file" logic. If multiple files all import a shared library like helpers.kcl or polygon.kcl then we'll want to avoid re-parsing them.

adamchalmers · 2024-10-14T20:55:32Z

src/wasm-lib/kcl/src/executor.rs

+                        if !module_exports.contains(&import_item.name.name) {
+                            return Err(KclError::Semantic(KclErrorDetails {
+                                message: format!(
+                                    "Cannot import \"{}\" from module because it is not exported. Add \"export\" before the definition to export it.",


Eventually (doesn't have to be in this PR) it'd be good to check for export-violation before we evaluate it -- just to return errors quicker.

I would love if we had a simple way to extract all the exported identifiers (and their type?) without evaluating anything. But yeah, I don't think we're there yet.

src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__af.snap

…tions

This reverts commit 282225c.

src/wasm-lib/kcl/src/ast/types.rs

src/wasm-lib/kcl/src/parser/parser_impl.rs

jtran requested review from nrc and adamchalmers October 13, 2024 19:06

jtran marked this pull request as ready for review October 13, 2024 19:21

vercel bot deployed to Preview October 13, 2024 19:24 View deployment

nrc added the kcl Language and compiler features label Oct 13, 2024

nrc reviewed Oct 14, 2024

View reviewed changes

adamchalmers reviewed Oct 14, 2024

View reviewed changes

jtran mentioned this pull request Oct 15, 2024

Rename things to be clearer about expression vs. value #4163

Merged

vercel bot deployed to Preview October 15, 2024 19:17 View deployment

jtran added 18 commits October 15, 2024 18:06

Add import of entire modules

f816ae0

Change to import module items, not the entire module

82e78f8

Add parsing import statements

4853449

Fix to check for cirularity before reading file

2883cae

Add project directory

5249df1

Fix TS to use ImportStatement

3044a4c

Add succeeding tests for simple import statements

be52d1c

Add export keyword

a17bf17

Add CodeMirror syntax highlighting for import and export

668a4fb

Fix parsing to allow import() calls

dc2cb09

Rename field to be clearer

24d5450

Add test for import cycle

3b45b1c

Add error when exporting non-function

60bf121

Fix to not allow interacting with engine during import

61bcbfa

Add skipped tests

0ab4bd7

Fix unparsing import statements and add tests

74b0533

Change to wrap in Box to appease lint

f907c3d

Fix to specify no visibility keyword when generating variable declara…

27e450e

…tions

jtran requested a review from nrc October 16, 2024 02:42

vercel bot had a problem deploying to Preview October 16, 2024 02:47 Failure

jtran added 5 commits October 15, 2024 23:25

Revert "Add explicit enum for project directory"

69c3cf3

This reverts commit 282225c.

Add docs explaining None

bb273ce

Update snapshots to use option for project directory

f5e1aef

Fix lint

ce7c1f9

Fix unit test since default value is no longer serialized

6cb50b3

vercel bot deployed to Preview October 16, 2024 04:20 View deployment

Merge branch 'main' into jtran/modules

1922be2

vercel bot deployed to Preview October 16, 2024 21:32 View deployment

Merge branch 'main' into jtran/modules

2fae126

vercel bot deployed to Preview October 16, 2024 21:57 View deployment

Code review

45c9552

adamchalmers approved these changes Oct 16, 2024

View reviewed changes

src/wasm-lib/kcl/src/ast/types.rs Outdated Show resolved Hide resolved

src/wasm-lib/kcl/src/parser/parser_impl.rs Outdated Show resolved Hide resolved

Remove redundant parsing code left behind from the import keyword

1ca27fb

vercel bot deployed to Preview October 16, 2024 22:24 View deployment

Change is_default function to be less general

eddbf61

vercel bot deployed to Preview October 16, 2024 22:44 View deployment

jtran enabled auto-merge (squash) October 16, 2024 22:57

adamchalmers disabled auto-merge October 17, 2024 04:48

adamchalmers merged commit 0577b6a into main Oct 17, 2024
25 of 32 checks passed

adamchalmers deleted the jtran/modules branch October 17, 2024 04:48

jtran mentioned this pull request Oct 17, 2024

Fix reading files from WebAssembly #4183

Merged

1 task

pierremtb mentioned this pull request Oct 17, 2024

Cut release v0.26.0 #4196

Merged

jtran mentioned this pull request Nov 7, 2024

Executor needs to know the KCL file name #4425

Open

franknoirot mentioned this pull request Nov 7, 2024

Cut release v0.26.3 #4427

Merged

This was referenced Nov 8, 2024

Should be an error to import anywhere besides the top level #4432

Open

Should be an error to have export anywhere besides the top level #4433

Open

Error message for import cycle detection is ordered strangely #4434

Open

jtran mentioned this pull request Dec 6, 2024

KCL: 'import' statements #4080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: KCL modules, part 1 #4149

internal: KCL modules, part 1 #4149

jtran commented Oct 13, 2024 •

edited

Loading

qa-wolf bot commented Oct 13, 2024

vercel bot commented Oct 13, 2024 •

edited

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading

nrc left a comment

nrc Oct 14, 2024

adamchalmers Oct 14, 2024

jtran Oct 15, 2024

jtran Oct 16, 2024 •

edited

Loading

adamchalmers Oct 14, 2024

adamchalmers Oct 14, 2024

adamchalmers Oct 14, 2024

adamchalmers Oct 14, 2024

jtran Oct 15, 2024

	#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, Bake)]
	#[databake(path = kcl_lib::ast::types)]
	#[ts(export)]
	#[serde(tag = "type")]
	pub enum BodyItem {
	ExpressionStatement(ExpressionStatement),
	VariableDeclaration(VariableDeclaration),
	ReturnStatement(ReturnStatement),
	}

internal: KCL modules, part 1 #4149

internal: KCL modules, part 1 #4149

Conversation

jtran commented Oct 13, 2024 • edited Loading

Important

Overview

qa-wolf bot commented Oct 13, 2024

vercel bot commented Oct 13, 2024 • edited Loading

codecov bot commented Oct 13, 2024 • edited Loading

Codecov Report

nrc left a 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

jtran Oct 16, 2024 • edited Loading

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

jtran commented Oct 13, 2024 •

edited

Loading

vercel bot commented Oct 13, 2024 •

edited

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading

jtran Oct 16, 2024 •

edited

Loading