Function scopes #1032

PgBiel · 2023-04-28T23:26:21Z

Closes #843.

Summary of changes

Adds function scopes, or a way for functions to double as a function and a module. This allows for some rather concise and attractive syntax for functions related to others. For example, this PR adds enum.item as a function under the enum function, allowing the creation of enum item objects to be able to manipulate their numbers. (Previously, this required using an array (number, body).)
This PR also adds assert.eq and assert.ne as samples, which assert if two values are equal and not equal, respectively. (The function names can be discussed, but that's the idea.)
Closures and user-defined functions do not have scopes (currently, at least). They are restricted to the standard library and cannot be implemented on the "typst-side".
You can also #import names from function scopes:

#import assert: eq, ne
eq(5, 5)
ne(10, 10)

#enum(..{
  import enum: *
  (
    item(1)[First],
    item(5)[Fifth],
    item(10)[Tenth]
  )
})

Also added tests for most things.

Implementation details

To add a scope to an internal function (#[func]) or element (#[element(...)]), use the syntax (in "Rust-side"):

#[func]  // or #[element(...)]
#[scope(
  scope.define("A", other_func);
  scope.define("B", SomeElem::func());
  scope
]
pub fn ...

A scope: Scope field was added to FuncInfo (shared by both native funcs and element funcs), which holds a function's scope.
The generation of this field was implemented in the #[func] and #[element(...)] proc macros. By default, it generates a Scope::new(). However, if you give it something like:

#[scope(
   scope.define("A", b);
   scope
)]

then it will generate:

FuncInfo {
  // ...
  scope: {
    let mut scope = Scope::deduplicating();
    scope.define("A", b);
    scope
  }
}

To achieve this, the struct FieldParser was renamed to BlockWithReturn and moved to util, as its very implementation was reused for this purpose. (The #[parse] attribute shares this parsing implementation of blocks with a returning expression.)
To get fields from a function's scope, Func::get was implemented.
To maintain support for .with and .where, method resolution code in eval/mod.rs was changed to prioritize a function's methods over its fields when doing function.something(). That is, function.with() and function.where() will call methods, while function.whatever() will fetch the field first.
~~For assert.eq and assert.ne to be able to use == and !=, the module eval.ops was made pub.~~ (Undone after review.)
src/ide/complete.rs was updated to consider function scopes in autocomplete.

Discussion points

For consistency with modules, #import func: * will always succeed (when func isn't a closure), even if the function's scope is empty. (Should it error if it has no scope fields/functions?)
Also for consistency, #import func will succeed and import the function's actual name to the scope. So, if enum is inside a dictionary's e key and you do #import dict.e, the enum name will be brought to scope. This behavior could be changed as needed.
While writing documentation for enum.item, assert.eq and assert.ne, I used the syntax (e.g.) [assert.eq]($func/assert.eq), but I'm not sure if this is ideal (e.g. could conflict with an option with the same name).
Names for assert.eq and assert.ne (as opposed to, e.g. assert.eq.not or something) are debatable.
I didn't remove the (number, body) syntax for enum items yet, as that would (I guess) be a breaking change; but please tell me if it's fine to remove it.

rpitasky · 2023-04-29T02:14:08Z

I am against “function scopes”, at least at the level that I understand them. I still think there’s a chance I’m misunderstanding something.

No other standard library I have ever seen does it this way. Every unusual thing we do is another weird thing a programmer has to learn, and if there are enough of these weird things the language feels clunky to use and learn. The best way to make a language easy to pick up is to lean into the patterns programmers already know, not add special cases for convenience. Having some sense of internal logical consistency makes it easier for programmers to figure out what is valid and what is not. Let’s look at some examples.

For the assert example, this should be separate functions: assert, assert_eq, and assert_ne. This is a logical extension matches what you see in Rust, for example. I don’t think it makes sense that assert is both a function and some kind of object/namespace/whatever. It doubly doesn’t make sense that I can’t make this sort of thing in native typst, so some illogical language magic is going on that I have to know and reason about every time I read this code.

The initial example of path given by @laurmaedje is worse, in my opinion. Not only do we have the function/namespace weirdness but it’s also stateful in a very real sense, something that we have a whole section of the language dedicated towards managing. I think it makes sense to have the draw.path function accept a lambda of some kind, where you are given an element/object (perhaps named “turtle” or something, it doesn’t matter at all really) and can call move/line/arc/whatever methods on it.

I hope I don’t come off as rude here, especially considering the significant amount of very-much-appreciated work here that @PgBiel has put into this, but I do not think this would be a positive change for the langauge.

PgBiel · 2023-04-29T02:36:39Z

No other standard library I have ever seen does it this way. Every unusual thing we do is another weird thing a programmer has to learn, and if there are enough of these weird things the language feels clunky to use and learn. The best way to make a language easy to pick up is to lean into the patterns programmers already know, not add special cases for convenience. Having some sense of internal logical consistency makes it easier for programmers to figure out what is valid and what is not. Let’s look at some examples.

I think some fundamental misconception is at work here. Function scopes work like static methods in classes - it's just that, in Typst, "classes" are elements, which boil down to functions (e.g. enum(), table(), ... - they are all elements, but always displayed as functions). So, if this helps, consider that we are adding "static methods" to the language, since elements already double as functions and "classes" (which are also namespaces in various languages). This will work not just to reduce global namespace pollution, but also to improve readability of certain functions.

For the assert example, this should be separate functions: assert, assert_eq, and assert_ne. This is a logical extension matches what you see in Rust, for example. I don’t think it makes sense that assert is both a function and some kind of object/namespace/whatever.

Relating to what I said above, this change is focused on elements, but extended to functions since both are very similar things in Typst. Now, note that this is consistent with the "Typst" way of doing things - instead of having tons of variables to indicate different things like in LaTeX (e.g. \neq, \rightarrow, leftarrow, ...), we have stuff like eq.not, arrow.r, arrow.l, and so on. Actually, you can even do both arrow() and arrow.r() (it doubles as a symbol and a function), so the base idea of function scopes is already in the language - just restricted to math symbols which can be specified above / below other things as well. So, by doing things this way, we are remaining consistent with the typst style - there is nothing too "new" here that you won't see in other parts of Typst already (this syntax already exists!).

It doubly doesn’t make sense that I can’t make this sort of thing in native typst, so some illogical language magic is going on that I have to know and reason about every time I read this code.

I actually think it's fair to demand to be able to do this from the typst-side, but this initial PR focuses on making the feature possible in the first place, so that other PRs may build on top of this.
However, saying that this is "illogical language magic" is going too far in my opinion - as long as this is properly documented (like all things), this isn't the end of the world at all. I don't know how you see this, but, for me, things like table.cell() or enum.item() are perfectly intuitive, and just another way of expressing this idea (other than having tons of parent-child style variables in the global scope).

Worth saying that this approach also has the advantage of being able to import all functions related to another within a scope, without much hassle. For example, if you're working with canvas, instead of having tons of functions like canvas-draw, canvas-node, etc., we can have canvas.draw, canvas.node, and then do:

#canvas[
  #import canvas: *
  #draw((1, 2), (3, 4))
  #node("a")
  #draw((5, 6), (7, 8))
  // ...
]

Sure, you could have a ctx => thing there, but, as Laurenz once said, this would make documentation harder, as the functions wouldn't actually be in any namespace! So it makes sense to have them as canvas.X.

The initial example of path given by @laurmaedje is worse, in my opinion. Not only do we have the function/namespace weirdness but it’s also stateful in a very real sense, something that we have a whole section of the language dedicated towards managing.

I think there was some miscommunication here - it's not any more stateful than it would be if it were called path-draw or something. It simply uses Typst return joining magic (the same magic that joins all arrays in a for loop that returns arrays). By joining in order, you know the order of each command in the path (it's how the PR currently implements it).

I think it makes sense to have the draw.path function accept a lambda of some kind, where you are given an element/object (perhaps named “turtle” or something, it doesn’t matter at all really) and can call move/line/arc/whatever methods on it.

As said above, this wouldn't do well for documentation, as the sub-functions wouldn't be in any namespace.

I hope I don’t come off as rude here, especially considering the significant amount of very-much-appreciated work here that @PgBiel has put into this, but I do not think this would be a positive change for the langauge.

I appreciate the feedback! But I think more consideration was needed regarding the language as a whole. This PR actually doesn't introduce any "new" syntax (as I comment above), and brings something that I believe is more intuitive for the users, overall. But we can discuss this further and add any needed improvements.

rpitasky · 2023-04-29T04:52:58Z

For one, I think it is uncharacteristically lazy to reject the obvious solution in favor of something so much less desirable just because the documentation generator isn’t quite good enough yet.

PgBiel · 2023-04-29T06:49:47Z

For one, I think it is uncharacteristically lazy to reject the obvious solution in favor of something so much less desirable just because the documentation generator isn’t quite good enough yet.

Okay, you do have a point - that by itself isn't enough to justify this addition. I think it boils down to a design choice in the end, just like all things in a language. But, at least for me, it makes sense for elements to have their own "sub-elements" here. At least this is the most valid use-case in my opinion - arguments can be held over the usage of scopes on more trivial things like assert, but at least for elements, it makes total sense to me to be able to e.g. table.cell, enum.item, ... It's just like an 'associated/static method/class', but in the context of this language.

To be clear, though - your concerns, by themselves, are valid, and by no means do I wish to diminish them. What's important to point out here is that, if you had those concerns, then new users can too. So I think we should make things clear since day-one - an explanation for this should be at least in the tutorial, if this is indeed added.

The main thing here is that, while I'd love to be able to implement scopes for user-defined functions in this PR, it faces the future-proofing issue: currently, functions have the .with and .where methods, so, if we let users define their own methods (over which we have no control), if we ever need to add more methods to functions, they'd necessarily represent a breaking change.
This could be solved by using a separate notation for scope resolution - such as Rust's :: - but this would require much more in-depth discussion, imo. So this is a bit complex of an equation to solve.

In the end, though, I just want to avoid having this discussion devolve into some sort of "fear of change" - if we all got used to new math syntax, then I think we can get used to this too (as such syntax even exists already anyway)... so the main focus of the discussion should be whether or not this feature can achieve its goal, of bringing some intuitive calls and also allowing easier usage of related functions in some contexts (such as path or canvas) - and, honestly, I think it does, but I am open to hearing different opinions.

PgBiel · 2023-04-29T09:41:10Z

Forgot to mention, but I also think calling it "lazy" is a bit exaggerated, no? Just making a context dict and writing the docs manually is much simpler than any of this... this is just a different solution, you know? And I thought it looked good, and several others did too, so I worked on it. Idk. Is it really that bad? 🤔

Like, I can't see how it would hinder language usability in any shape or form. I think that being able to keep auxiliary functions/"classes" inside their parents enables us to make public a lot more of the internals without polluting the global namespace. This includes the examples I mentioned (table.cell, enum.item, ...). Sure, you could have a module like "tables" or one like "enums" but isn't that a bit redundant? And a bit inconsistent since table and enum wouldn't be in those modules. Just group them together, and boom! Table.cell is the cell for a table, and enum.item is the item for an enum, and there you have it! It just kinda clicks to me...

laurmaedje · 2023-04-29T11:35:17Z

For one, I think it is uncharacteristically lazy to reject #673 (comment) in favor of something so much less desirable just because the documentation generator isn’t quite good enough yet.

This was just an additional consideration. The main point is that I don't think that it's less desirable. The function scopes are more flexible than the closure callback (for instance, in the closure you can't mutate variables from outside, which you might want). And aside from the drawing thing, it generalizes well to other parts of the language and is consistent with symbol notation. But @PgBiel argued all of this already much better than I would've done.

PgBiel · 2023-04-29T15:38:24Z

The function scopes are more flexible than the closure callback (for instance, in the closure you can't mutate variables from outside, which you might want).

Yes! That is something I forgot to mention, but it's very very important - closures are less flexible than simple blocks.

laurmaedje · 2023-04-29T18:34:45Z

library/src/compute/foundations.rs

+    #[default]
+    message: Option<EcoString>,
+) -> Value {
+    if !typst::eval::ops::equal(&expected, &actual) {


!typst::eval::ops::equal(&expected, &actual) is the same as expected != actual, see here

Oooh, I missed that, thanks.

laurmaedje · 2023-04-29T18:36:33Z

library/src/compute/foundations.rs

+/// Returns:
+#[func]
+pub fn assert_eq(
+    /// The expected value.


I wouldn't call one "expected" and one "actual". I, for one, more frequently write them in the other order in Rust. Rust just calls them left and right. This of course also affects the error message.

Alright! Is the new message fine?

laurmaedje · 2023-04-29T18:36:43Z

library/src/compute/foundations.rs

+    /// The actual value.
+    actual: Value,
+
+    /// An optional message to display on error


all comments should wrap at 80 columns

laurmaedje · 2023-04-29T18:36:57Z

library/src/layout/enum.rs

+/// customize the number of each item in the enumeration:
+/// ```example
+/// #enum(
+///     enum.item(1)[First step],


2 spaces of indent :)
also in other places.

library/src/layout/enum.rs

laurmaedje · 2023-04-29T18:42:33Z

src/eval/mod.rs

@@ -1077,7 +1086,14 @@ impl Eval for ast::FuncCall {
            } else {
                let target = target.eval(vm)?;
                let args = args.eval(vm)?;
-                if !matches!(target, Value::Symbol(_) | Value::Module(_)) {
+
+                // Prioritize a function's own methods (with, where) over its fields.


no need to duplicate the comment, bad enough that the whole code is a bit duplicate here (not your fault)

laurmaedje · 2023-04-29T18:43:40Z

src/eval/func.rs

+                )
+            }),
+            Repr::Closure(_) => Err(eco_format!(
+                "cannot access fields on closures and user-defined functions"


"closure" isn't a term that is exposed to the user.

Suggested change

"cannot access fields on closures and user-defined functions"

"cannot access fields on user-defined functions"

laurmaedje · 2023-04-29T18:43:52Z

src/eval/mod.rs

-                }
+        if let Value::Func(func) = source {
+            if func.info().is_none() {
+                bail!(span, "cannot import from closures or user-defined functions");


Suggested change

bail!(span, "cannot import from closures or user-defined functions");

bail!(span, "cannot import from user-defined functions");

src/eval/mod.rs

laurmaedje · 2023-05-03T12:20:47Z

The docs generation will of course need to be updated. I'll take care of that before the release.

laurmaedje · 2023-05-03T12:20:58Z

Thanks!

PgBiel added 19 commits April 27, 2023 21:57

initial funcscopes impl

7ae09b7

apply fmt

842b4ec

prioritize func methods over fields

4ae0159

make function scope non-optional + simplify .get()

4244710

missing formatting

0c49830

add import from function scope

6cde980

add 'with_scope' macro instead of 'scope_builder'

def1062

use 'scope' instead of 'with_scope'

4a2d4a2

fix scope macro hygiene

01e5c33

add enum.item

582edea

add assert.eq and assert.ne

acc20fd

add function scope import tests

6205737

add functions to import error message

097d7eb

fix import() formatting

4018e8c

add assertion tests

ed06e70

add enum.item tests

402be43

improve comments on method call resolution

4effafb

slight docs improvement

12bc092

add function field access tests

2be7d07

update field completion in src/ide

37a893d

laurmaedje reviewed Apr 29, 2023

View reviewed changes

PgBiel added 2 commits April 29, 2023 15:58

use value equality instead of exposing ops

ce697b3

remove 'expected' and 'actual' from asserts

6da97c2

PgBiel added 3 commits April 29, 2023 16:09

deduplicate 'create_scope_builder' in macros

9be5869

fix indenting on typst examples & tests

0ff553c

improve comments and messages

3d2330f

LaurenzV mentioned this pull request Apr 29, 2023

Add support for date & time handling #435

Merged

epbuennig mentioned this pull request May 2, 2023

Conversion between Unicode codepoint values and chars #1068

Closed

Merge branch 'main' into func-scopes

0579614

laurmaedje merged commit f88ef45 into typst:main May 3, 2023

PgBiel deleted the func-scopes branch May 4, 2023 02:54

epbuennig mentioned this pull request May 7, 2023

Unicode code point conversion (#1068) #1132

Merged

flavioabar pushed a commit to flavioabar/typst that referenced this pull request May 23, 2023

Function scopes (typst#1032)

712402b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function scopes #1032

Function scopes #1032

PgBiel commented Apr 28, 2023 •

edited

Loading

rpitasky commented Apr 29, 2023 •

edited

Loading

PgBiel commented Apr 29, 2023

rpitasky commented Apr 29, 2023

PgBiel commented Apr 29, 2023 •

edited

Loading

PgBiel commented Apr 29, 2023 •

edited

Loading

laurmaedje commented Apr 29, 2023

PgBiel commented Apr 29, 2023

laurmaedje Apr 29, 2023

PgBiel Apr 29, 2023

laurmaedje Apr 29, 2023

PgBiel Apr 29, 2023

laurmaedje Apr 29, 2023

laurmaedje Apr 29, 2023

laurmaedje Apr 29, 2023

laurmaedje Apr 29, 2023

laurmaedje Apr 29, 2023

laurmaedje commented May 3, 2023

laurmaedje commented May 3, 2023

	"cannot access fields on closures and user-defined functions"
	"cannot access fields on user-defined functions"

	bail!(span, "cannot import from closures or user-defined functions");
	bail!(span, "cannot import from user-defined functions");

Function scopes #1032

Function scopes #1032

Conversation

PgBiel commented Apr 28, 2023 • edited Loading

Summary of changes

Implementation details

Discussion points

rpitasky commented Apr 29, 2023 • edited Loading

PgBiel commented Apr 29, 2023

rpitasky commented Apr 29, 2023

PgBiel commented Apr 29, 2023 • edited Loading

PgBiel commented Apr 29, 2023 • edited Loading

laurmaedje commented Apr 29, 2023

PgBiel commented Apr 29, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurmaedje commented May 3, 2023

laurmaedje commented May 3, 2023

PgBiel commented Apr 28, 2023 •

edited

Loading

rpitasky commented Apr 29, 2023 •

edited

Loading

PgBiel commented Apr 29, 2023 •

edited

Loading

PgBiel commented Apr 29, 2023 •

edited

Loading