Dart 3 null safety - #1213
Conversation
Add shell.nix to install dart2 and zulu, nix will already have a c compiler
> allow debug C builds when tooling injects _FORTIFY_SOURCE
|
fixes #1212 |
|
I'm not sur if the updates in pubspec.yaml make this harder than it would/could be, but it feels like a good thing to do and |
|
I didn't include any of the generated changes to site/, I wasn't sure if that was appropriate, but it would also mean you don't have to clone this branch in order to see those changes. Possibly this was outlined in a contributing.md, and I just didn't see it yet |
.dart try to do something better than !
|
Thank you, this worked for me and got me unblocked! Before After |
| left: -(math.div($col, 2)); | ||
| width: math.div($col, 2); |
There was a problem hiding this comment.
nit: if possible, would prefer this PR to have been split into multiple parts instead of all boilerplate in 1 (thought not sure if that's possible). Ex:
- migration to
@use "sass:math"; - CSS hexcodes --> hsl
- Non functional formatting changes
There was a problem hiding this comment.
Yeah, I kind of felt the same way about this one. It was mixed. I can work on splitting this up into the dart pacakges as one update, and the dart 3 in another. I can't remember if I started getting errors on one of the packages and that is why I went ahead and updated them, or if this just started to feel like a modernization story to me and I updated the packages. These were just warnings that they would break in the future at any rate. I try not to have warnings emitted with a build. But this isn't really related to the nullability safety at all.
There was a problem hiding this comment.
Got it, thanks for context. I feel you, thanks for fixing them. Since I've already begun reviewing I'm cool with continuing, just a heads up for future reference!
| BUILD_SNAPSHOT := $(BUILD_DIR)/build.dart.snapshot | ||
| TEST_SNAPSHOT := $(BUILD_DIR)/test.dart.snapshot | ||
|
|
||
| DART_FLAGS := --old_gen_heap_size=1024 |
There was a problem hiding this comment.
On an early version of dart3, it will build fine without this. Prior to pushing this, I updated to a more recent dart 3, and the build started hanging with an OOM error. Adding this flag allowed it to once again build successfully. You could try with DART_FLAGS := "" and see if you can replicate it, or if it is needed. I don't use dart a whole lot, so I'm not sure if this was maybe something transient introduce in one version and then fixed, or if it will carry forward to future versions.
There was a problem hiding this comment.
Got it thanks, can we add 1-line code comment so this is easier for people in future to know why it was set?
There was a problem hiding this comment.
Since this is not required to unblock the main issue (Dart 3 dep), this should be in it's own separate PR. Being able to run this requires installing nix package manager. While this can be helpful, it also adds more steps for getting started, so tradeoffs for that can be discussed separately
There was a problem hiding this comment.
I based this PR onto my other PR which pinned the dart version at dart 2. Being able to have a locked down version of DART was kind of key to being able to validate this migration, because I could then compare the output of the dart2 to the output of dart3, and make sure I didn't accidentally introduce a regression. I know a lot of folks don't use nix, but this is why it was there. I had made the prior PR, because it would only build under dart2, and I had to spend a bit of time figuring out how to get an old version up and going. nix package manager is supported on many distros. Using a dockerfile could get a similar experience, and maybe be useful to more people. But I use nix.
#1199 // Dart 2 shell.nix PR
There was a problem hiding this comment.
Thanks for context, sure I'm ok with adding it since it's additive
| _location = _location.parent ?? (throw StateError('Location parent is null')); | ||
| } | ||
|
|
||
| // Module variables are only a single line. | ||
| if (_location.kind == "variable") { | ||
| _location = _location.parent; | ||
| _location = _location.parent ?? (throw StateError('Location parent is null')); | ||
| } | ||
|
|
||
| // Hack. There is a one-line class in Parser.java. | ||
| if (line.contains("class ParseError")) { | ||
| _location = _location.parent; | ||
| _location = _location.parent ?? (throw StateError('Location parent is null')); |
There was a problem hiding this comment.
would prefer to have centralized helper methods like getParent() where we only need to do null-check once. In general would prefer to do this instead of having to inline every callsite
There was a problem hiding this comment.
I do more c# than dart by far. This is maybe a c# pattern than isn't as used?? But yes, adding a getParent would mean less boilerplate. Initially I did this with ! which also works, but I felt like the exception would be slightly better information. But also maybe the runtime ! failure would have given similar info and a stack trace. I was trying not to do any other refactoring as part of this PR, because I'm not supre familiar with this code base. But I do think this is good feedback, and I'll see if I can get to making this change when I strip out the dependency updates.
There was a problem hiding this comment.
Oh yea, I think ! would be good for this case. Or also doing a check for _location.parent at the top of the flow
| var startChapterName = match[1] ?? (throw ArgumentError('Start chapter missing in match')); | ||
| var endChapterName = match[3] ?? (throw ArgumentError('End chapter missing in match')); |
There was a problem hiding this comment.
Yea same thing, just some private helper like _assertNotNull(value, "var_name")
There was a problem hiding this comment.
Not sure if I agree on this one, but I'll give it some thought. To me this feels the same as _assertNoNull, but maybe I'm not getting what you are meaning by your comment. I think that for what you are sugesting, I may as well juse to match[1]! which is what I did on the first pass, but I thought having a more specific error message mya be helpful.
There was a problem hiding this comment.
Yea I think just doing ! is helpful too. Anyways, not a huge blocker, just easier to read
rossdancraig
left a comment
rossdancraig
left a comment
There was a problem hiding this comment.
Thanks for doing this and getting people like me unblocked!
Overall lgtm just would prefer to split this up into parts that fix the dep issue vs. parts that don't (ex: nix script) and also centralize a few helpers, feel free to ping me again once ready for review again and I'll approve!
|
In general, I preferred |
rossdancraig
left a comment
rossdancraig
left a comment
There was a problem hiding this comment.
Cool I'm down to stamp, looks good enough to fix core issues where Lox can't be used with Dart 3 so hopefully this gets people unblocked. Feel free to message me if you decide to make any changes and need another review!
2 participants
No description provided.