Update unnamed instances changelog entry#4122
Conversation
I know this has already gone out, but the current treatment of this feature in the changelog is slightly inaccurate - it's not true that these numbers are randomly generated, they should actually be deterministic - if the module hasn't otherwise changed, you should always get the same number out. I don't think we should document the scheme for what generated instance names look like, because we shouldn't consider it part of the compiler's public API; documenting it could be read as telling users that they can rely on this not changing, which is not the case.
CHANGELOG.md
Outdated
| and the compiler will generate a unique name for the instance. Note that | ||
| generated instance names contain numeric suffixes which can change without | ||
| warning, and should therefore not be relied on. |
There was a problem hiding this comment.
As long as we're pedantically revising history, since #4096, the generated names might not have a suffix at all, so even this is technically inaccurate. I might go even further and avoid mentioning any explanation for how generated names can change:
| and the compiler will generate a unique name for the instance. Note that | |
| generated instance names contain numeric suffixes which can change without | |
| warning, and should therefore not be relied on. | |
| and the compiler will generate a unique name for the instance. Note that | |
| generated instance names can change between builds, and should therefore | |
| not be relied on. |
There was a problem hiding this comment.
This sounds to me like leaving everything else the same, if you compile, delete your output directory, and then compile again, you might get a different generated name, which isn’t true, right?
There was a problem hiding this comment.
But #4096 isn’t in any released version so far anyway, correct?
There was a problem hiding this comment.
Correct on both counts, but:
- Do we want to go into that level of detail if the takeaway is ‘don't rely on this’?
- I think ‘can change without warning’ also allows for the same interpretation (delete and recompile could change it).
- The revised wording isn't incorrect pre-Use GenIdent for anonymous instances #4096, just less necessary. I don't know who would be best served by a description of an internal detail that we expect will change in the next release and that we want people to ignore anyway.
There was a problem hiding this comment.
Can we just change this to
and the compiler will generate a unique name for the instance. Do not use or rely on the instance name in FFI.
Correct me if I'm wrong, but the above comment was only to clarify that if one were to use it in FFI, it could cause problems because the name could change.
There was a problem hiding this comment.
Something that draws the line between ‘don't do this’ as a matter of principle and ‘don't do this because’ as a practical warning does seem valuable.
There was a problem hiding this comment.
Perhaps this?
and the compiler will generate a unique name for the instance. Do not use or rely on the instance name in FFI as relying on it can produce runtime errors in some situations.
There was a problem hiding this comment.
I don't really like that either; it can be irritating to be told that something can cause errors "in some situations" but not be given any clue as to what those situations are.
|
Is this something we still want to change? If so, what's the next action? |
|
I'd still like to change it. I think we are fairly close to consensus here. How about
which I think balances the concerns of wanting to avoid having too much detail as this is an internal thing, but also giving enough detail to be able to explain why it's a bad idea to rely on them, and also avoids suggesting that these names are non-deterministic. |
|
It might be worth clarifying that we're talking bout not relying on depending on the name in the FFI, as now the revised version doesn't mention that it might seem like we're talking about something in PureScript code? |
|
So something like this?
|
|
I'm thinking we close this PR. It hasn't seen any activity for a while. |
|
I think your last proposal is good and I think we should run with it rather than close this out. |
|
Done. Just waiting for CI to build before we'll merge this. |
4 participants
I know this has already gone out, but the current treatment of this feature in the changelog is slightly inaccurate - it's not true that these numbers are randomly generated, they should actually be deterministic - if the module hasn't otherwise changed, you should always get the same number out. I don't think we should document the scheme for what generated instance names look like, because we shouldn't consider it part of the compiler's public API; documenting it could be read as telling users that they can rely on this not changing, which is not the case.
Description of the change
Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.
Checklist: