Skip to content
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

varpartitions: add support for if/try/case paths #1455

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

alaviss
Copy link
Contributor

@alaviss alaviss commented Sep 10, 2024

Summary

This commit adds basic support for paths in conditional cases. Conditionals are considered valid paths when all branches of execution that does not terminate execution yields the same path.

Details

  • info that couldn't fit into the summary
  • relevant details; tricky parts
  • anything else

Fixes full_issue_url


Notes for Reviewers

  • Right now this is for my own experimentation, so progress is gonna be slow as I play around with this.

Todo list:

  • Write tests
  • Update experimental docs

This commit adds basic support for paths in conditional cases.
Conditionals are considered valid paths when all branches of
execution that does not terminate execution yields the same
path.
@alaviss alaviss added enhancement New feature or request compiler/sem Related to semantic-analysis system of the compiler labels Sep 10, 2024
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some initial thoughts

break samePathCheck

let remainderStart = if n.kind == nkCaseStmt: 2 else: 1
for idx in remainderStart ..< n.len:
Copy link
Collaborator

@saem saem Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any trailing finally should be removed from the iteration since it's not part of the expression, as in it will always be a statement, right?

Comment on lines +384 to +385
# Only check branches with a type, as no types implies noreturn for
# expressions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're either noreturn or unit, the difference being that the latter doesn't "abort" the current expression like the former does. You might need to rely on the endsInNoReturn analysis.

@@ -362,6 +362,40 @@ proc pathExpr(node: PNode; owner: PSym): PNode =
break
else:
break
of nkElifExpr, nkElifBranch, nkElseExpr, nkElse, nkExceptBranch, nkOfBranch:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this dig into nkBlock expression and statement varieties?

@zerbina
Copy link
Collaborator

zerbina commented Sep 10, 2024

In my opinion, extending the path expression specification to have if cond: x else: x be considered a path expression is unnecessarily (in terms of implementation effort) and also not very useful).

A simpler yet more useful solution would be to allow block/if/case/try expressions in a view creation context. Implementation-wise, for varpartitions, this would amount to just adding the following procedure to varpartitions:

proc borrowFromComplex(c: var Partitions; dest: PSym, src: PNode) =
  ## Borrows from all relevant sub-expressions of `src`.
  case src.kind
  of nkStmtListExpr, nkBlockExpr, nkExceptBranch:
    borrowFromComplex(c, dest, src[^1])
  of nkIfExpr:
    for it in src.items:
      borrowFromComplex(c, dest, it[^1])
  of nkCaseStmt:
    for (_, it) in branches(src):
      borrowFromComplex(c, dest, it[^1])
  of nkTryStmt:
    for i in 0..<(src.len - ord(src[^1].kind == nkFinally)):
      borrowFromComplex(c, dest, src[i])
  of nkHiddenAddr:
    # currently necessary to skip the outermost hidden-addr
    borrowFromComplex(c, dest, src[0])
  elif endsInNoReturn(src):
    discard "nothing to do"
  else:
    borrowFrom(c, dest, src)

and then changing

borrowFrom(c, dest.sym, src)
to

borrowFromComplex(c, dest.sym, src)

borrowFrom is additive, so to speak, so calling it multiple times means that multiple locations are borrowed from. This nets you support for the let x: lent T = if cond: a else: b case.

You'll still get a run-time or compiler crash when doing so, since if, block, etc. are npt expected to appear as an nkHiddenAddr operand. I can quickly make a PR with the necessary preparations, if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/sem Related to semantic-analysis system of the compiler enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants