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

add decls.byLent, turns an expression into a let #14875

Closed
wants to merge 5 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 1, 2020

add decls.byLent which turns an argument into a let-param; one use case is forcing a let param overload.
this was kind of blocked by
now that these lent bugs have been fixed (#14578 and #14557 and #14576), this is worthwhile.

  • see examples in this PR
  • see also this example:
when true:
  import std/decls
  proc fun(y: int): int = result = 10
  proc fun(y: var int) = y = 10
  proc main()=
    var it = 0
    it = fun(it.byLent) # without byLent, would give: Error: expression 'fun(it)' has no type
  main()

which is a reduction of the only test failure I'm getting in #14869, for package https://github.com/Vindaar/ggplotnim

2020-07-01T10:34:35.1020690Z /Users/runner/.nimble/pkgs/ginger-0.2.5/ginger.nim(1358, 40) Error: expression 'updateScale(view, it)' has no type (or is ambiguous)

using byLent is the correct way to handle this IMO

@timotheecour timotheecour changed the title add decls.byLent for let-param overload resolution add decls.byLent, helps to force a let-param overload Jul 1, 2020
@timotheecour timotheecour changed the title add decls.byLent, helps to force a let-param overload add decls.byLent, turns an expression into a let Jul 2, 2020
@timotheecour timotheecour requested a review from Araq July 2, 2020 06:18
@Araq
Copy link
Member

Araq commented Jul 2, 2020

Meh, before allowing lent in more places we should implement borrow checking.

@timotheecour
Copy link
Member Author

I can't find an example where it produces unsafe code (of the form: stack variable escapes its stack frame). eg, this works:

when true:
  proc byLent2[T](a: T): lent T =
    result = a
  proc baz: auto = [3,4]
  proc main()=
    let s0 = [12,13]
    let s = byLent2(baz())
    let s2 = [12,13]
    doAssert s == [3,4]
  main()

but even if such examples existed, byLent is still useful; without it it's unclear how you'd deal with procs that overload a var vs let param such as normalizePathEnd (see let nimblePath = conf.nimblePaths[i].string.byLent.normalizePathEnd in this PR in compiler/options.nim)

@Araq
Copy link
Member

Araq commented Jul 2, 2020

The way normalizePathEnd is overloaded is wrong indeed and needs to be fixed.

@timotheecour
Copy link
Member Author

timotheecour commented Jul 2, 2020

I don't like having to use/remember arbitrarily different names like:
normalizePathEnd vs normalizedPathEnd (if we'd rename it)
sort vs sorted (in std/algorithm)
because there's nothing systematic about it (english grammar...) and it creates 2N API's.

that leaves these 2 options:

I'd be fine with standardizing on option 2, but it'd require the ./ operator sugar instead of dup for several reasons (I just need to finish that WIP but the code worked and had performance benefits)

@timotheecour
Copy link
Member Author

timotheecour commented Jul 8, 2020

Meh, before allowing lent in more places we should implement borrow checking.

@Araq PTAL, byLent now takes a var input now that you've fixed #14878 (thanks!) which was blocking this; this should remove concerns on memory safety of byLent (even if I still couldn't find an example where the previous version would be unsafe, see #14875 (comment))

normalizePathEnd vs normalizedPathEnd is a separate topic altogether

@Araq
Copy link
Member

Araq commented Jul 9, 2020

normalizePathEnd vs normalizedPathEnd is a separate topic altogether

No, it's not. It was a mistake to add to os.nim, it misuses overloading and needs to be deprecated. In fact, we need compiler support to prevent this from happening again. Overloading by var T is for allowing two different views into a container's element. It's for nothing else. The non-var normalizedPathEnd shouldn't exist anymore anyhow thanks to dup.

@Araq
Copy link
Member

Araq commented Jul 9, 2020

And btw, the fact that these procs lack the export marker was super confusing.

since((1, 1)):
  export normalizePathEnd

is also not code that should be supported, export is for forwarding, not for avoiding the export stars!

@stale
Copy link

stale bot commented Jul 9, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jul 9, 2021
@stale stale bot closed this Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants