-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 forward progress update to NEWS.md #54089
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e795d44
Update NEWS.md for forward progress
LilithHafner c3a76d6
Remove emoji
LilithHafner 162862c
Clarify that only trivial infinite loops were UB
LilithHafner b46c3bd
Answer @adienes's FAQ
LilithHafner 09f9653
Merge branch 'master' into LilithHafner-patch-1
LilithHafner a3708f7
Grammar
LilithHafner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Can we get rid of the "Undefined Behavior" jargon? Julia — as I understand it — should not have UB.
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.
It's not jargon, it's precise. There's many constructs in julia that are UB - incorrect
@inbounds
, various poking at internal data structures, etc.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.
In particular, this is not a bugfix, it's a specification change for the language (even though we don't have the specification written out).
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.
In my version of the unwritten spec this was a bug, though! :p
Seriously, though, unlike
@inbounds
, etc., there were no documented caveats to whatwhile true; end
meant. Invalid uses of@inbounds
and internals muckery are clearly warned to be invalid and break things. That is something different than UB to me.I maintain that UB is a compiler-writer's jargon — it has very specific understandings and expectations. It means not just that a result might be arbitrary/unspecified, but it means that the compiler can do wild and unexpected things because of that. It's why we
freeze
values from LLVM.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.
If it had been a bug, we would have just fixed it years ago and wouldn't needed #40009 to discuss the language semantics change. Would it be nice to have a more formal spec written down somewhere about what is or is not undefined behavior in the language? Sure! But in the meantime, you can ask the people who implemented the thing about what decisions were made about the language semantics. And in this case, the decision was originally to inherit LLVM's memory model with all that that implies and then we reconsidered that decision in #40009, which is what's being documented here. Once that decision was made #52999 indeed became a bugfix, but that bug fix doesn't really need NEWS. We fix way worse compiler bugs than that all the time. What needs NEWS is that we changed the semantics of the language.
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.
Retroactively update the unwritten Julia spec (unwritten specs are easy to update) so that this is in fact a bugfix and then it doesn't need a NEWS entry at all and just document the current "policy" somewhere.
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.
Sigh, sure, but the same could be said about everything. The purpose of NEWS is to give people a heads up about changes that they may need to be aware of. Changing the forward progress guarantees means that people are likely to see perf regressions in cases where we previously were relying on these semantics, may need to update assume effects, may need to update external compiler passes, etc. That's the purpose of documenting changes in language semantics, so that when people upgrade to 1.12 and they see something weird here, they may remember reading about it in NEWS. That's also why I think it's important to use the precise and correct terminology here, which does include "undefined behavior" and "forward progress" so that people who need to care about it can use it to find the extensive related literature. The change of behavior for
while true; end
is the least interesting part of this (because people don't usually write code that way anyway). I don't really get what the problem is with just writing down the change in language semantics that we made.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.
I wrote some devdocs on this: #54099. I still think the NEWS entry needs to be precise about what the actual change is.
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.
I'm happy to include precise technical language if you provide a link to a webpage documenting what that language means (looking at the term "forward progress" here). The news should be accessible to folks who don't have the C spec nor Julia's unwritten spec memorized.
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.
Yes, this is exactly at the crux of why I'm suggesting these changes. The important thing to me is how Julia's behavior changes, because that's all we have to go on.