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

[Diagnostics] Add an edu note explaining @_nonEphemeral diags #31649

Merged
merged 1 commit into from
May 12, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented May 8, 2020

The goal here is:

  • explain how ephemeral pointers can be introduced
  • note where they can and cannot be used
  • point users in the direction of the withUnsafe* APIs

Suggestions on how to improve the explanations and/or provide better guidance on best practices are welcome, this isn't really my area of expertise. Once SR-12739 is done we should be able to link to that documentation from here as well.

@owenv owenv requested a review from hamishknight May 8, 2020 01:45

Temporary pointers may only be passed as arguments to functions which do not store the pointer value or otherwise allow it to escape the function's scope. The Swift compiler is able to diagnose some, but not all, violations of this rule. Misusing a temporary pointer by allowing it to outlive the enclosing function call results in undefined behavior.

When calling functions which cannot safely accept temporary pointer arguments, use the `withUnsafe*` family of pointer APIs to explicitly manage pointer lifetimes.
Copy link
Collaborator

@xwu xwu May 8, 2020

Choose a reason for hiding this comment

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

This cries out for an example, or a link to a more elaborate explanation (e.g., the documentation for the APIs mentioned?), or both. I'd wager that there's approximately a 0% chance that someone who is learning about the topic anew from this education note will know what to do based on the dozen words here ("use ... to explicitly manage pointer lifetimes"). We've got a beautiful multi-paragraph explanation of the problem being diagnosed, and then when it comes to any possible solutions, it just...ends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll need to come up with some kind of simplified example here, even if it ends up being a bit contrived. The issue with linking to the documentation here is that it's hosted only on developer.apple.com, and there's no way of knowing when and if those links will change. It's the main disadvantage of shipping these notes in the toolchain.

Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Looks fantastic, thank you for writing this up!

userdocs/diagnostics/temporary-pointers.md Outdated Show resolved Hide resolved
userdocs/diagnostics/temporary-pointers.md Outdated Show resolved Hide resolved
userdocs/diagnostics/temporary-pointers.md Show resolved Hide resolved
@hamishknight hamishknight requested a review from atrick May 8, 2020 05:01

Temporary pointers may only be passed as arguments to functions which do not store the pointer value or otherwise allow it to escape the function's scope. The Swift compiler is able to diagnose some, but not all, violations of this rule. Misusing a temporary pointer by allowing it to outlive the enclosing function call results in undefined behavior.

When calling functions which cannot safely accept temporary pointer arguments, use the `withUnsafe*` family of pointer APIs to explicitly manage pointer lifetimes.
Copy link
Contributor

@atrick atrick May 8, 2020

Choose a reason for hiding this comment

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

Thanks for doing this! I think it's great and agree with all the feedback so far.

When calling functions which cannot safely accept temporary pointer arguments, use the withUnsafe* family of pointer APIs to explicitly manage pointer lifetimes.

Mentioning withUnsafePointer opens up a dangerous door. I think it's the biggest issue with the current warnings. The usual reaction will be to think that merely using withUnsafePointer imbues code with some extra safety--of course it doesn't, it just silences warnings. The educational note is the perfect place to clear this up.

If we leave it as "use withUnsafePointer to manage lifetime" without explaining how to "manage lifetimes", we'll just get a lot of this sort of thing:

struct PointerWrapper {
  var ptr: UnsafePointer<Int>
}

func foo(x: Int) -> PointerWrapper {
  return withUnsafePointer(to: x) {
    PointerWrapper(ptr: $0)
  }
}

It may help just to search through some common Swift packages to see common cases of programmers using withUnsafePointer correctly, and contrast that with incorrect usage.

@owenv
Copy link
Contributor Author

owenv commented May 10, 2020

I ended up adding a more in-depth example of incorrectly initializing an UnsafePointer with UnsafePointer(&x), along with good and bad examples of using withUnsafePointer to fix the problem. After doing a bit of research it looks like this is by far the most common mistake when using ephemeral pointers. Since this note is already kind of long, I'm hesitant to add more examples unless their are concerns this is still insufficient

Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Edits look great!

userdocs/diagnostics/temporary-pointers.md Outdated Show resolved Hide resolved
@xwu
Copy link
Collaborator

xwu commented May 11, 2020

Agree, this looks great!

@owenv
Copy link
Contributor Author

owenv commented May 11, 2020

@swift-ci please smoke test

@owenv owenv merged commit 6c646ec into swiftlang:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants