-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Purity inference #7170
base: main
Are you sure you want to change the base?
Purity inference #7170
Conversation
9039954
to
c063025
Compare
Moves the "STATEMENT AFTER EXPRESSION" error from the parser to canonicalization. We'll later use this to allow this case in effectful functions.
This reverts commit 52896d9fa65141df832989b326f526cbedf67341. We actually still need this for when `!` follows a non-ident expr
This is because the NoIdentifiersIntroduced error was moved to the type checker.
c063025
to
ee94d81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review, only partway through. More comments tomorrow!
crates/compiler/can/tests/snapshots/test_suffixed__suffixed_tests__apply_argument_multiple.snap
Outdated
Show resolved
Hide resolved
|
||
match loc_expr.value { | ||
Var(symbol, _) if symbol.is_builtin() => { | ||
// NOTE: This assumes builtins are not effectful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you add List.walk!
? Would that break this?
Maybe what you mean is that builtins don't use Stmt(_)
, which is why you removed the !
usage from one of the Task
functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment refers to builtins that would produce the effect, not to higher-order functions that would just call it. The latter should be fine because when inlined, they would still include the call to the real effectful function and the enclosing function would be correctly categorized as such.
Effect.putLine! "You entered: $(line)" | ||
Effect.putLine! "It is known" | ||
|
||
forEach! : List a, (a => {}) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan on adding List.walk!
or even List.forEach!
to the std lib in this PR? If not, we'll want to make an issue to do so. I believe we decided in Zulip we'd like one or both of those in the meantime while for
loops are cooking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've finished reading the whole thing (more or less). It looks good to me! A couple of minor things, and then I think we're good to go. You supporting both Tasks and effectful functions makes this need much less coordination to get right than the built-in Task effort.
const BANG_FLAG: u32 = 1u32 << 31; | ||
const UNSUFFIXED: u32 = !BANG_FLAG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really smart idea! And we still have about 2 billion idents per module, so we should have plenty of space haha
alloc.reflow("If you don't need any arguments, use an empty record:"), | ||
]), | ||
alloc.parser_suggestion(" askName! : {} => Str\n askName! = \\{} ->\n Stdout.line! \"What's your name?\"\n Stdin.line! {}"), | ||
alloc.reflow("This will allow the caller to control when the effect runs."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a heuristic for when to use singular or plural for effect(s)? This error seems to be singular on purpose in contrast to other errors here.
I have been leaning towards almost always pluralizing because you don't know how many effects will run when you call effectful functions.
let stack = [ | ||
alloc.reflow("This assignment doesn't introduce any new variables:"), | ||
alloc.region(lines.convert_region(region), severity), | ||
alloc.reflow("Since it doesn't call any effectful functions, this assignment cannot affect the program's behavior. If you don't need to use the value on the right-hand-side, consider removing the assignment.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I think it's right-hand side
@@ -10828,55 +10806,7 @@ All branches in an `if` must have the same type! | |||
Foo = Foo | |||
"# | |||
), | |||
@r" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the current change doesn't seem to report any issues when pure expressions are destructured into zero new assigned values. I think this is undesired, though I like the general way you're handling statements now. Do you think there's a way to fix your canonicalization code to handle this without just re-introducing the NoIdentifiersIntroduced
code?
@r###" | ||
── UNNECESSARY EXCLAMATION in /code/proj/Main.roc ────────────────────────────── | ||
This matches a pure function, but the name suggests otherwise: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term "matches" here makes sense once you understand that function args are patterns we are "matching" on, but otherwise is a little clunky. However, I can't think of a better way to phrase it...
Maybe "This variable should be a pure function, but..."? It seems like this "matches" term will always show the region of a captured variable, but I may be mistaken there. But if that's the case, I definitely prefer referring to the variable instead of the underlying concept of pattern matching.
@agu-z can you ping me for review after rebase? |
Introduces the "Purity Inference" model of effects as described in Richard's talk:
!
and viceversa!
suffix is not only required on defs, but also on record fields, and all pattern matched identifiers (tuple, tags, opaques, etc).IGNORED RESULT
warning is produced if they return anything other than{}
_
:roc run
a program with a pure/effectful mismatch, which can be useful while you're debuggingTask
still works if that's what the platform exposes in itshosted
module. This will allow us to migrate platforms incrementally.Type checking and errors preview:
https://www.youtube.com/watch?v=5a3r88-oEQQ