Anthony Bullard (Jan 02 2025 at 03:33): Anthony Bullard (Jan 02 2025 at 03:33):I think that PNC should land probably tomorrow - unless Joshua takes another look and has another point of contention. I have a stash with all examples and builtins migrated to use PNC(I moved the flag to an option that allow the user to choose a migration to apply (all, none, snake_case, and parens_and_commas). Should I attempt to do the Tutorial as well? Any thing else I should address as a fast follow?
Sam Mohr (Jan 02 2025 at 03:36):The tutorial would be a good change, but a different PR would be good, yes
Anthony Bullard (Jan 02 2025 at 03:37):Oh yeah, this will all be in a separate PR for sure
Sam Mohr (Jan 02 2025 at 03:37):I don't think there's anything that needs to be in a fast followup
Sam Mohr (Jan 02 2025 at 03:37):Since this change will be made incrementally
Sam Mohr (Jan 02 2025 at 03:38):But updating the builtins next would unblock deprecation of whitespace function calls
Sam Mohr (Jan 02 2025 at 03:39):I presume you have a mental list of the steps between now and getting this into basic-(cli|webserver) releases
Sam Mohr (Jan 02 2025 at 03:39):Happy to talk through those if you don't
Sam Mohr (Jan 02 2025 at 03:39):But that's the main goal, after which case we do either soft deprecation via warnings or hard deprecation via removal
Sam Mohr (Jan 02 2025 at 03:40):But I understand removal to be necessary to unblock other syntax features, so I think we should just bite that bullet
Anthony Bullard (Jan 02 2025 at 03:44):Screenshot 2025-01-01 at 9.44.43 PM.png
Sam Mohr (Jan 02 2025 at 03:46):Hmm... not a perfect intermediate state, but it's correct, so go for it!
Anthony Bullard (Jan 02 2025 at 03:47):Here's my checklist
Anthony Bullard (Jan 02 2025 at 03:48):Sam Mohr said:
Hmm... not a perfect intermediate state, but it's correct, so go for it!
This was done by hand editing HTML in dev tools. What's off to you?
Sam Mohr (Jan 02 2025 at 03:48):The syntax itself
Anthony Bullard (Jan 02 2025 at 03:48):Because no new lambda syntax and SD?
Sam Mohr (Jan 02 2025 at 03:48):Not a you thing, an evolution by discrete generations
Sam Mohr (Jan 02 2025 at 03:48):Yeah
Anthony Bullard (Jan 02 2025 at 03:49):Ah, I'd actually be very happy with this
Anthony Bullard (Jan 02 2025 at 03:49):But I really would like to do the new lambda syntax next
Sam Mohr (Jan 02 2025 at 03:49):I mainly dislike the odd (but correct) alignment of the parens for List.map(...)
Sam Mohr (Jan 02 2025 at 03:49):otherwise this looks good to me
Anthony Bullard (Jan 02 2025 at 03:50):Let me see how we would format this right now
Sam Mohr (Jan 02 2025 at 03:50):That's why I was pushing for that weird Kotlin syntax that takes final argument callbacks out of the parens, but they don't seem to be popular, so :shrug:
Anthony Bullard (Jan 02 2025 at 03:53):Oh that's exactly how it will look after migration
Sam Mohr (Jan 02 2025 at 03:53):Yep
Anthony Bullard (Jan 02 2025 at 03:54):Add a comma to the end of the lambda, and you get:
credits = List.map(
songs,
\song -> "Performed by $(song.artist)",
)
Why the double indent?
Anthony Bullard (Jan 02 2025 at 03:55):That's my fault copy/pasting
Sam Mohr (Jan 02 2025 at 03:55):Oh, okay
Anthony Bullard (Jan 02 2025 at 03:55):To format it, I had to wrap it in a function:
does_not_matter = \songs ->
credits = List.map(
songs,
\song -> "Performed by $(song.artist)",
)
credits
I see
Anthony Bullard (Jan 02 2025 at 03:56):Because I was using a real file :-)
Anthony Bullard (Jan 02 2025 at 03:56):Could have done it as a snapshot but thought that would be silly
Brendan Hansknecht (Jan 02 2025 at 04:18):Since this will lead to breaking changes, I think it would be good to slip #7448 in with it. Both require updates to platforms for functionality to continue.
Brendan Hansknecht (Jan 02 2025 at 04:19):Also, I would advise pushing [ ] Update basic-cli / basic-webserver platforms right after Update builtins and definitely before the annoucement. Otherwise, people will read the announcement but not really be able to test anything cause basic-cli won't be updated yet.
Anthony Bullard (Jan 02 2025 at 11:58):This is just PNC migration - not a breaking change for the platform as long as they are using nightly compiler
Brendan Hansknecht (Jan 02 2025 at 13:35):Ah, this won't include the snake case renaming. I thought we were shipping both together.
Anthony Bullard (Jan 02 2025 at 14:17):Migrating builtins to snake_case _is_ breaking and a lot of work
Anthony Bullard (Jan 02 2025 at 14:17):Well, relative to migrating to PNC
Anthony Bullard (Jan 02 2025 at 14:20):One thing that I'm noticing in this migration that I'm not sure if we want is that crash, dbg, and try are looking like application instead of a keyword [space] expr sequence in a statement
Anthony Bullard (Jan 02 2025 at 14:22):I know that the plan is to deprecate try keyword once we have PNC (is that correct @Sam Mohr ), so i'm not as worried about that, but crash and dbg I don't think should look like application even if that technically works
Sam Mohr (Jan 02 2025 at 14:31):Yes, I believe it's better to have those keywords be visually distinct
Anthony Bullard (Jan 02 2025 at 14:35):It's a formatting issue that I'll dive into before I have to go back to work :cry:
Brendan Hansknecht (Jan 02 2025 at 14:35):I assumed they would also transfer to PNC.
Brendan Hansknecht (Jan 02 2025 at 14:35):I mean they aren't distinct in today's roc.
Brendan Hansknecht (Jan 02 2025 at 14:35):So why make them distinct in PNC?
Brendan Hansknecht (Jan 02 2025 at 14:36):They of course can be colored differently by the lsp
Anthony Bullard (Jan 02 2025 at 14:36):Yeah, it's a small but important question.
Anthony Bullard (Jan 02 2025 at 14:36):Let me check return
Brendan Hansknecht (Jan 02 2025 at 14:36):Also, dbg needs to fit into method syntax pipelines.
Brendan Hansknecht (Jan 02 2025 at 14:37):.pass_to(dbg) or .(dbg)()
Anthony Bullard (Jan 02 2025 at 14:37):Ok, return does what I expect which is it is a distinct keyword in a statement
Brendan Hansknecht (Jan 02 2025 at 14:39):Yeah, return is a true keyword. dbg and crash could easily be platform functions instead as dbg! and crash!. that is why I don't think they should be too special
Anthony Bullard (Jan 02 2025 at 14:39):I don't think we want them marked as effectful do we?
Anthony Bullard (Jan 02 2025 at 14:40):That would color any otherwise pure function that uses them
Sam Mohr (Jan 02 2025 at 14:40):Without syntax highlighting they aren't distinct as special.
Sam Mohr (Jan 02 2025 at 14:40):I think he's just pointing out that they could be implemented as platform functions, not that they should have exclams
Sam Mohr (Jan 02 2025 at 14:40):Since they're so simple
Anthony Bullard (Jan 02 2025 at 14:40):Sure
Anthony Bullard (Jan 02 2025 at 14:41):But is the intermediate state OK?
Anthony Bullard (Jan 02 2025 at 14:41):Where they are in the language still special keywords, but formatted like function application?
Anthony Bullard (Jan 02 2025 at 14:43):Oh, and I just noticed that Sam landed his PR removing backpassing
Anthony Bullard (Jan 02 2025 at 14:43):So....we'll see if I get around to dealing with all of those conflicts today
Brendan Hansknecht (Jan 02 2025 at 14:44):Yeah, sorry if my comment was confusing. Was just trying to say I think we should handle dbg and crash like normal functions. They really are normal platform functions. They just get a special exception not to require !
Anthony Bullard (Jan 02 2025 at 14:56):Makes sense!
Anthony Bullard (Jan 02 2025 at 14:56):That actually will simplify the compiler quite a bit
Anthony Bullard (Jan 02 2025 at 14:57):Makes me wish that return could just be a unary op like ^ and then I don't think we would need statements at all anymore
Sam Mohr (Jan 02 2025 at 15:02):I agree, but since return is only used at the start of statements, it's not that bad
Anthony Bullard (Jan 02 2025 at 15:03):Yes but now it will be the only thing requiring us to having statements
Sam Mohr (Jan 02 2025 at 15:03):Doesn't x = 123 also require that?
Anthony Bullard (Jan 02 2025 at 15:04):What I mean is now Statements can just be a enum of {BindingStmt, ExprStmt}
Anthony Bullard (Jan 02 2025 at 15:05):And technically it wouldn't be needed since an Expr block would just be something like a enum of {ExprOnly(expr), WithBindings(bindings: [Binding], expr: Expr)}
Sam Mohr (Jan 02 2025 at 15:06):Sure, but I don't think a grammar of
(pattern "=" | "return")? expr
for statements is that bad
Anthony Bullard (Jan 02 2025 at 15:06):True
Sam Mohr (Jan 02 2025 at 15:06):We're probably on the same page here. Would love to remove it, not sure how :shrug:
Anthony Bullard (Jan 02 2025 at 15:07):And I guess ^ would eat into the strangeness budget even though (or maybe exactly because) it's cribbed from SmallTalk :tears:
Brendan Hansknecht (Jan 02 2025 at 15:36):it's cribbed from SmallTalk
never would have guessed. I just thought it was something totally random.
Anthony Bullard (Jan 02 2025 at 23:27):PNC is gonna have to wait a day or two. I pulled in @Sam Mohr 's change and now there is one snapshot that is failing in a nasty way and I gotta figure out what happened.
Sam Mohr (Jan 02 2025 at 23:28):Is there something I'd be able to help with?
Anthony Bullard (Jan 02 2025 at 23:28):Maybe
Anthony Bullard (Jan 02 2025 at 23:33):This:
C(4#
)implements e:m
C
is supposed be:
@0-22 SpaceAfter(
Defs(
Defs {
tags: [
EitherIndex(0),
],
regions: [
@0-20,
],
space_before: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
space_after: [
Slice<roc_parse::ast::CommentOrNewline> { start: 0, length: 0 },
],
spaces: [],
type_defs: [
Ability {
header: TypeHeader {
name: @0-1 "C",
vars: [
@2-3 SpaceAfter(
NumLiteral(
"4",
),
[
LineComment(
"",
),
],
),
],
},
loc_implements: @6-16 Implements,
members: [
AbilityMember {
name: @17-18 "e",
typ: @19-20 BoundVariable(
"m",
),
},
],
},
],
value_defs: [],
},
@21-22 SpaceBefore(
Tag(
"C",
),
[
Newline,
],
),
),
[
Newline,
],
)
but comes out instead as:
@0-22 SpaceAfter(
Defs(
Defs {
tags: [
EitherIndex(2147483648),
],
regions: [
@0-20,
],
space_before: [
Slice<roc_parse::ast::CommentOrNewline { start: 0, length: 0 },
],
space_after: [
Slice<roc_parse::ast::CommentOrNewline { start: 0, length: 0 },
],
spaces: [],
type_defs: [],
value_defs: [
Annotation(
@0-6 Apply(
@0-6 Apply(
@0-1 Tag(
"C",
),
[
@2-3 SpaceAfter(
NumLiteral(
"4",
),
[
LineComment(
"",
),
],
),
],
ParensAndCommas,
),
[
@6-16 Identifier {
ident: "implements",
},
@17-18 Identifier {
ident: "e",
},
],
Whitespace,
),
@19-20 BoundVariable(
"m",
),
),
],
},
@21-22 SpaceBefore(
Tag(
"C",
),
[
Newline,
],
),
),
[
Newline,
],
)
When you say "is supposed to be", do you mean that's the old state, or do you mean that's what you personally think it should be?
Anthony Bullard (Jan 02 2025 at 23:34):I think it's meant to be a Ability, but this "expr", even formatted makes zero sense to me at all
Anthony Bullard (Jan 02 2025 at 23:35):I didn't think you could have implements in the header of an annotation
Sam Mohr (Jan 02 2025 at 23:35):I think this test isn't super useful
Anthony Bullard (Jan 02 2025 at 23:36):Here's what it supposedly is formatted:
C 4 #
implements e : m
C
Okay, that makes more sense, now that I've referenced the ability docs
Sam Mohr (Jan 02 2025 at 23:40):No, I think that your new behavior is correct
Anthony Bullard (Jan 02 2025 at 23:40):The closest logical thing would be:
C a #
implements e : m
C
Really?
Sam Mohr (Jan 02 2025 at 23:40):Oh, actually
Sam Mohr (Jan 02 2025 at 23:40):So we're supposed to have two parsing formats
Anthony Bullard (Jan 02 2025 at 23:40):Because if you remove the comment/newline you get
C a implements e : m
C
Types have their args applied with spaces
Sam Mohr (Jan 02 2025 at 23:41):But values have them applied with parens
Anthony Bullard (Jan 02 2025 at 23:41):Yes
Sam Mohr (Jan 02 2025 at 23:41):So your code is parsing it like a value instead of a type
Anthony Bullard (Jan 02 2025 at 23:41):And that will remain the case after PNC
Anthony Bullard (Jan 02 2025 at 23:42):Oh, so it's the order that we look for defs then right?
Sam Mohr (Jan 02 2025 at 23:42):That might be it
Sam Mohr (Jan 02 2025 at 23:42):I'm looking now
Anthony Bullard (Jan 02 2025 at 23:43):It seems like if we find the a Expr::Var("implements") or Pattern:Identifier("implements") we should Err out with a malformed...
Sam Mohr (Jan 02 2025 at 23:44):I'm not sure I agree, but maybe
Sam Mohr (Jan 02 2025 at 23:45):There is logic to say any ident that matches a keyword is blocked
Sam Mohr (Jan 02 2025 at 23:46):But we don't include IMPLEMENTS in KEYWORDS
Sam Mohr (Jan 02 2025 at 23:46):I'd add that keyword and see if it breaks
Sam Mohr (Jan 02 2025 at 23:46):Or rather, that this parses correctly
Anthony Bullard (Jan 02 2025 at 23:46):Ok, I'll try the same
Anthony Bullard (Jan 02 2025 at 23:47):But this I think is because we only parse an ability if the first expression in the expression chain is a tag
Anthony Bullard (Jan 02 2025 at 23:47):Now with parens, it will always be an Apply
Sam Mohr (Jan 02 2025 at 23:48):Man, I'm flip-flopping over here
Anthony Bullard (Jan 02 2025 at 23:48):So this is similar to how we have to accept that SomeTag(1,2) is equivalent to SomeTag 1 2 and never SomeTag (1,2)
Sam Mohr (Jan 02 2025 at 23:49):I think this case is testing a behavior that makes sense if PNC doesn't exist, but makes less sense in PNC land. Maybe this is crazy, but I think you should just delete this
Anthony Bullard (Jan 02 2025 at 23:49):I'm OK with it if you are. I'll make sure that
C 4 #
implements e : m
C
Give the right AST
Anthony Bullard (Jan 02 2025 at 23:51):Yeah it does - with a little swapping around of Spaces
Anthony Bullard (Jan 02 2025 at 23:51):I can rename it to not mention parens and just overwrite
Anthony Bullard (Jan 02 2025 at 23:59):Rebase complete
Anthony Bullard (Jan 03 2025 at 00:02):Now here's hoping I can get an approval before someone lands something else in the parser/formatter :fingers_crossed:
Sam Mohr (Jan 03 2025 at 00:05):Looking now
Anthony Bullard (Jan 03 2025 at 00:09):Shoot, I have a can test failure
Anthony Bullard (Jan 03 2025 at 00:09):Looks like a couple were updated by you Sam, I just gotta add the PatternAppyStyle
Sam Mohr (Jan 03 2025 at 00:10):Okay, still reviewing
Anthony Bullard (Jan 03 2025 at 00:11):Done
Anthony Bullard (Jan 03 2025 at 00:11):I love insta
Sam Mohr (Jan 03 2025 at 00:12):It makes life much easier and brainless
Anthony Bullard (Jan 03 2025 at 00:14):Can we move test_syntax to this please?
Sam Mohr (Jan 03 2025 at 00:15):If you make a PR, I'll approve it
Anthony Bullard (Jan 03 2025 at 00:15):I can try sometime soon
Anthony Bullard (Jan 03 2025 at 00:15):I have like 3 other issues queued up :-)
Anthony Bullard (Jan 03 2025 at 00:16):Luckily Joshua went over this pretty good, so the review should be relatively easy
Anthony Bullard (Jan 03 2025 at 13:08):This PR is now only blocked by a failed panic check. I added a new .unwrap() in the minimize binary in test_syntax during arg parsing. I believe that is reasonable behavior (and following the local pattern). If someone with authority to override it could take a look I would be very thankful.
Anton (Jan 03 2025 at 13:10):I replied in the PR
Anthony Bullard (Jan 03 2025 at 13:13):Just tagged you in a comment on the line
Anthony Bullard (Jan 03 2025 at 13:14):I replaced
let text = std::fs::read_to_string(&args[index + 1]).unwrap();
with
let text = if input == "-" {
std::io::stdin().read_to_string(&mut buf).unwrap();
buf.to_string()
} else {
std::fs::read_to_string(input).unwrap()
};
I'm going to write a Roc program that can actually show you the panics and unwraps added in a feature branch
Anthony Bullard (Jan 03 2025 at 14:24):Actually you really just need to change the violations collection line to:
VIOLATIONS="$(cargo clippy --no-deps -- -W clippy::unwrap_used -W clippy::expect_used -W clippy::panic 2> >(grep -C 1 -e "warning: \`panic\`" -e "warning: used" -e "warning: usage of" | grep -e " --> " | sed "s/ *--> *//g"))"
Then compare the number of violations
NUM_SOURCE_VIOLATIONS="$(echo "$SOURCE_VIOLATIONS" | wc -l)"
NUM_TARGET_VIOLATIONS="$(echo "$TARGET_VIOLATIONS" | wc -l)"
if [ "$SOURCE_VIOLATIONS" -gt "$TARGET_VIOLATIONS" ]; then
...
echo "Here are new warnings I found in the target branch:"
comm -13 source_branch_dir/violations target_branch_dir/violations
fi
That may lead to false positives from moved files (like during a refactor) but that could be an acceptable shortcoming
Anthony Bullard (Jan 03 2025 at 14:35):It would at least narrow down your search
Anthony Bullard (Jan 03 2025 at 14:35):And we could make the language have more hedging :smile:
Anthony Bullard (Jan 03 2025 at 14:36):echo "Here are some location that **may** have introduced new violations in the target branch:"
I wanted to do something much more ambitious with Roc, like using histogram diff and parsing the patch to find actual net new lines of code. But this is a step forward I think
Anton (Jan 03 2025 at 14:40):Agreed :)
Anthony Bullard (Jan 03 2025 at 15:32):I'll do this when I'm done with the rebase I'm working on - which will be after work today :-)
Last updated: Jul 06 2025 at 12:14 UTC