-
Notifications
You must be signed in to change notification settings - Fork 33
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
KCL: 'use' statements #4080
Comments
As stated, there are a few implications of the above that might be unintuitive.
To address those latter points, I think we should make this minor adjustment:
If not immediately, shortly afterwards, we should make this change also:
|
This feels like a good step towards a library of standard parts that we can supply! I'm thinking various screw sizes (M3, M4, etc.). To address @jtran concerns my suggestion would be something akin to C/C++ header guards (#pragma once or the C #ifndef) this prevents multiple imports of the same module and prevents infinite I don't know if we have scope within kcl but For the problem of side effects you could enforce that anything that is going to be a module can't execute anything? Only declare variables and functions. Then it becomes more of a design to make a module be a separate entity from an "executable" kcl. First line of the file has to include I like lua's import where you specify a directory and it pulls in all of the files within the directory |
Yes it seems we should restrict to importing a module once. Multiple import looks to bring more problems than it's worth. +1 for the whole proposal! |
Sorry, I didn't spell it out anywhere, but I assumed that |
It sounds nice, to only allow them at the top level. But if I guess in the short-term, if you're just using your own files, it's fine. We can always add it to conditionals later if we find we need it. (Side-effects ruin everything 😢) |
If there is a project.toml in the same directory as the file being imported it should be executed with those units, if there is not it should be “unitless” meaning it uses whatever the current project is that’s my only request , this looks great otherwise |
@jessfraz plan is to only import files within the same project for now (we can remove this limitation later) wdyt |
@benjamaan476, I proposed that the limitation is that a |
I think that’s fine but I’m just saying it’s stupid that kcl-samples works with the CLI just fine because we traverse directories for the project.toml but it’s stupid. It doesn’t work with the app and it’s stupid that no one can just load it as its own project in the app but it works fine with the CLI so I’m just saying it would be easy to implement on the CLI side a.k.a. in the pure Rust code so maybe we just do it but up to you. another thing to keep in mind when implementing this but out of scope for the first pass is, we are going to also want to do use thing.json as data and so if there’s a way to implement it, Such that future us doesn’t have pain that would be great |
I’d like to move away from the whole project dynamic, but there’s a separate issue for that. Just things should work no matter where you open it no matter how many nested directories it has. |
Sorry, I mostly mentioned this just so that we don’t implement this in a way such that we are shoehorned in to the whole project thing. It’s fine if it only works for projects now, but I just don’t want to implement it such that removing this whole concept fucks us later Right now the CLI a.k.a. the pure right side works really nicely wherever the fuck you are |
@jessfraz what do you mean "kcl-samples works with the CLI", I've never opened kcl-samples, how does it work with the CLI? I assume you mean something beyond just "you can snapshot and export KCL files from kcl-samples"? |
I'd like to understand the motivation in more depth - why do users want to use multiple files? Is it because files are so large they are unwieldy? Because they want to share code between projects? Because they want to divide a program up for the sake of modularisation (and in that case what is the motivation? Documentation? Abstraction?)? Something else? There seems to be some assumption around the other files being secondary in some sense - that they should not be modified by the UI and so forth. Could you give me some more context on where that comes from? With the major caveat that I don't understand the context nor the existing system well enough to really have opinions, my opinions are:
|
My understanding is that the main motivation is users wanting to make multiple parts that share helper functions and constants, and they don't want to have to copy and paste that helper code all over. Currently, people generally make one KCL file per part. If you put multiple parts in one file, this is nice for viewing in the app. But when you export that part, it will be multiple volumes in a single glTF, and that may or may not work in the program where you're consuming it. The second reason is if you have multiple projects that happen to do something similar, you also want to share code between them. As projects get bigger, this is becoming more common. A simple example is implementing a function for the Zoo logo. The third reason, which I believe is what triggered all this happening now, is that there's some design space that is hard to get experience with and explore without multiple files. In the conversation about project.toml and units #3960, for example, people had a hard time saying what they even wanted when mixed units were involved, partly because there's currently no way to use multiple files. It was concluded that we needed a simple way to share code sooner rather than later. Early proposals suggested doing an expedient approach of a C-style include that added the included file's definitions to the file where it was included. There are a lot of problems with this, and I pushed for some semblance of a module so that the semantics of a KCL file do not change depending on how it's used or error out due to name collisions.
It's not that we don't want to be able to modify But separate from all the above, in some ways, there is a fundamental difference between library code that is depended on and code that is run. When you run a script, you run it for its side-effects, like drawing geometry. But if you
👍 I think I like this, but do you have another suggestion? I thought about this issue too, and I figured that the current proposal is compatible with the future addition of something like:
Because it's a bare identifier, not a quoted string, we could distinguish it. But I generally agree that despite the "one file === one module" principle, it should still be possible for a module to exist without any file, like in the web app where there is no filesystem. As for file names in other encodings, I think it's fine to just say, sorry, we don't support that. I think the general principle of making unintended things be errors that we maybe relax later is a good approach. The |
Some more thoughts on this since it is becoming higher priority. I've been thinking about design choices which give us maximal choice for future growth along with some other tweaks.
Do functions need to be declared before use? I guess we should follow that logic.
|
Unfortunately, a lot of the rationale for this proposal didn't make it to this GitHub issue.
👍
We already have
This came up also. Basically, I think that very soon, people would get tired of listing all the items. Without editor tooling, it's busywork to have to update your imported items every time you add something to other.kcl. I'd anticipate users requesting I suppose this comes down to whether we value being able to statically determine whether a member access is defined. I definitely flip-flop back and forth on how static I think KCL should be. It's true that having the identifier listed explicitly makes static analysis simpler. But without its type, which presumably you'd need to analyze the other file anyway to get, is it really that much better? In the proposal, they'd be using I also find it's easier to read code that's prefixed with the module.
I think that someone said that we could make the breaking change of requiring I like this, but I think others will reject it. One thing to consider is that KCL projects are likely going to be small for a while, and public-by-default is really only a problem when you start sharing packages and having software engineering problems. OTOH, the skeptical part of me is saying, "will we ever really make the breaking change later?" It would have to be prioritized against everything else. Why not just do it from the start?
This would help reduce the needed difference between a top-level script and a library and generally make things easier for us implementers. But this is not how people write KCL today. It's common for people to define a bunch of numeric constants at the top of their file. I suppose we could tell them to just wrap it in a function. So this:
becomes
I suppose that can be a bit more concise if we implement the alternative
But now, on the calling side, people need to use Another approach is to use the object syntax.
This could be good when you have a lot of variables. I'm not sure what to say. I want to like this, but it's a little unsatisfying, and I'm afraid others won't go for it either. Could we have some kind of value restriction? Like a constant is possible only if it can be statically determined to be a constant? But this rules out
Yes, the change to this was agreed upon.
Functions currently need to be declared before use. I think it would be nice to relax this at some point, but we're not there yet. So I think the above should behave the same and error with
Yes, we agreed this would cause an error. As a user, I absolutely hate how Go disallows circular imports. Rust's approach is so much more convenient, in my opinion. But I'm not going to argue over this on the first version. Starting out strict is generally good.
That's the plan. Yes, if we required exporting only functions, it would simplify things. But I think we'd still want to do memoization to not have to reparse and reload the functions every time. But at that point, no user could ever tell the difference, which is good. It would be purely an internal optimization.
Yes
If we went with importing each function/item individually, not having renaming would lead to name collisions where you couldn't import some combinations of things.
I'm not sure I follow you here. We currently treat symbolic constants the same as a function call when doing code modifications in the sense that we treat it as fully constrained. Only a literal is treated as unconstrained.
👍 |
I think we mostly agree on the eventual goal, just perhaps not on the steps to get there. To be clear I'm trying to identify the absolute most minimal first iteration, not the 1.0 version - I don't know what that will look like, but I think we should make the smallest possible step first in order to have the maximum flexibility for related features and to get feedback as soon as possible (and also to maximize available time for other priorities.
My feeling was that eventually we would use import expressions for the role of the import function (that function is extremely magic otherwise). In the meantime, it is easy enough technically (albeit a little confusing) to allow import to be used as both a keyword and an identifier (we don't want to assign to import in any case). This is very much meant to be a temporary measure though! I'm not to married to the name though. We could use
I think this is just about starting with the simplest thing and seeing what people complain about, rather than guessing what they will complain about. We can add glob imports before 1.0 easily enough, but let's start as small as possible. I will say that glob imports bring a lot of problems - they're pretty much universally regretted in Rust. I think it is better to encourage individual imports or paths as names, but I'd like to see the problems real users hit before committing to a path forward. Also, we should absolutely implement editor support for imports - it's pretty easy and really useful. I'd rather prioritise that for 1.0 rather than more sophisticated import statements.
I strongly believe we should postpone implementing paths as names. Not that we shouldn't do it, just that we should think it through and iterate on the design before implementing. It's a difficult topic with subtleties. Starting from the syntax - using
I agree we want to be able to import contants in the long run. But for a first iteration, I think avoiding the side effects issue makes it worthwhile to forbid it. As you point out, consts can always be made into functions (like we do in std with
Later, later :-) Again there's lots of overlap with other reasons to treat modules like objects, so lets design that properly before implementing for the sake of this use case.
👍 sounds good
I'm kind of keen to see the kinds of name clashes that happen irl, it's possible that might inform our design some how? On the other hand, implementing renaming is pretty easy and I'm 99% sure we want it, so if you want to implement in the first iteration, go for it! Or it could be in the first PR after that to make the first one quicker to land.
Hmm, so maybe it won't help too much. I was mostly thinking just importing functions is easier than dealing with importing top-level geometry in terms of expectations. But I don't understand exactly what codemods can touch (I was assuming that code on the right-hand side of an assignment is more mutable some how than code in a function, but that might well be incorrect). Anyway, not too important for now :-) |
Okay, you've convinced me. Thank you, everyone, for all the input. |
Addresses #4080. (Not ready to close it yet.) # Important Requires a fix for #4147 before it can work in ZMA. # Overview ```kcl // numbers.kcl export fn inc = (x) => { return x + 1 } ``` ```kcl import inc from "numbers.kcl" answer = inc(41) ``` This also implements multiple imports with optional renaming. ```kcl 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: - #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.
For future work, I started a design doc at KittyCAD/kcl-experiments#12 |
Problem: Currently all KCL projects have to be a single file. There's no way to reuse code across files.
Solution: KCL files should be able to import each other, so that one file can use functions or constants defined in another.
Principles
One file === one module, and the directory tree (on disk) === the module tree (in code). This isn't really relevant right now but is a key principle we should stick to going forwards. I'm basing this on chats with various Rust maintainers around Rust's module system being overcomplicated and how we'd all like to simplify it.
Hygiene: importing a module should not change the behaviour of your code. This means it shouldn't overwrite your code's constants/functions and it shouldn't change your model. The only changes should happen when you actually use the code you're importing.
Design
Syntax
We add a
use
statement, with an optionalas
suffix. For example:This executes lens.kcl and makes its top-level identifiers (e.g.
fn cube()
) available under the namespacelens
. Your KCL file can now invokelens.cube()
like a normal KCL function.If you leave off the
as lens
part, it'll use the filename as the module name. For example, these two are equivalent:To make this easier, ZMA will stop letting KCL filenames contain spaces. Most CAD apps don't allow spaces in some contexts anyway SolidWorks, Siemens Sinumerik). The mechanical engineers agree with this decision.
Usage
A
use
statement is only valid in the main/top-level scope of the file. In other words, you cannot use it inside a function or any other expression.While importing a file, the KCL interpreter won't let it send any engine requests. This prevents side-effects when importing. Instead, it returns an error, saying "Imported files cannot execute API requests, consider wrapping this in a function instead, so that the caller can choose when it executes."
Initially we'll only support importing files from the same project (we'll remove this limitation eventually).
Implementation
When you import a file, it'll create a namespace in the importer. So, we'll add a new kind of KclValue variant,
KclValue::Namespace(HashMap<String, KclValue>)
. The HashMap contains items in the namespace, mapping their name to their value.When KCL interpreter executes a
use
statement, it'll:lens.kcl
from the ZMA projectkcl_lib::types::ast::Program
ast::Program
into key-value pairs in aKclValue::Namespace
Future extensions
use (eyepiece) from "lens.kcl"
which lets you use theeyepiece
symbol without prefixing it aslens.eyepiece
use "telescope/eyepiece.kcl"
)The text was updated successfully, but these errors were encountered: