-
Notifications
You must be signed in to change notification settings - Fork 676
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
No more "strong mode" #694
Conversation
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.
Overall, looking quite good! My main quibbles are about the rather loose use of "sound" instead of "type safe" (please see inlined comments). Maybe I'm being too pedantic. @eernstg could you chime in?
|
||
The simplest way to enable strong mode is to specify | ||
`strong-mode: true` in the analysis-options file: | ||
To find places in your code that could use explicit types, |
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.
Instead of
To find places in your code that could use explicit types,
how about either
To find places in your code that lack explicit type annotations,
or simply
To find places in your code that are implicitly typed,
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 didn't want to imply that you should explicitly type everything. It's fine to lack explicit type annotations, as long as the inferred type isn't (unintentionally) dynamic
, right?
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 that case, implicit-casts
isn't relevant here. Those are perfectly fine and won't infer dynamic
.
So, what is the purpose of this section, now that it's no longer telling you how to enable strong mode?
The section promotes the part of the original that tells you how to enable more checks, to be a full section, but without the initial part, that feels out-of-place.
So, maybe a lead-in like:
You can enable more static checks on top of the Dart 2 type system,
to tell you where the type system would allow a down-cast by inserting a run-time check,
or where the type inference cannot find a useful type and defaults to inferringdynamic
.
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, that is totally fine, and will be the preference of some developers
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 would phrase this along the lines of "You can enable additional flags to do more checks statically. To statically find places in your code where a downcast might result in a runtime error, set implicit-casts
to false; and to make inference report errors in places where it is unable to infer a type other than dynamic, set implicit-dynamic
to false".
@@ -146,43 +133,27 @@ String s = o; // Implicit downcast | |||
String s2 = s.substring(1); | |||
{% endprettify %} | |||
|
|||
This flag defaults to `true`. | |||
|
|||
`implicit-dynamic: <bool>` | |||
: A value of `false` ensures that the type inference engine never chooses |
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.
"never chooses dynamic" tells us what it doesn't do. We haven't yet in this subsection said what happens when these flags are set to false. We should state that an issue is reported (I'm not sure what the default issue is, but in any case I think that it can be changed via configuration ... or maybe that is only for the linter rules).
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.
Yeah, I noticed it was vague, but I wasn't trying to fix existing issues.
@@ -146,43 +133,27 @@ String s = o; // Implicit downcast | |||
String s2 = s.substring(1); |
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.
How about embedding the actually analyzer issue that is reported? (Similar to what is done elsewhere in the docs. I can help with this if you'd like.)
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 should learn how to do this. Maybe you could walk me through it tomorrow or Friday sometime?
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.
Certainly. Here is an example: https://github.com/dart-lang/site-www/pull/694/files#r176273967
src/_guides/language/index.md
Outdated
How and why to write sound Dart code, and how to use strong mode to | ||
enable soundness. | ||
<li markdown="1"> [Dart's Type System](/guides/language/sound-dart)<br> | ||
Learn about Dart's sound static type system. |
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.
Drop "static" (since soundness only makes sense in the context of a static type system).
@@ -1,54 +1,51 @@ | |||
--- | |||
title: Strong Mode Dart | |||
title: Dart's Type System |
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.
How about "Type Safe Dart"?
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 think we wanted to move toward this being a page about the type system, in general. Otherwise, I might've gone back to its first name, "Sound Dart".
Opinions, @anders-sandholm / @mit-mit / @mjohnsullivan?
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 really like the title 'Dart's Type System' and I think it's a nice general title for a page that covers an overall overview of how the typing system works.
@@ -183,35 +145,26 @@ A sound type system has several benefits: | |||
code is much less efficient. |
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 the previous sentence:
While AOT compilation is possible without
strongtypes, the generated
code is much less efficient.
src/_guides/language/sound-dart.md
Outdated
Dart's strong mode implementation, which enables soundness, consists | ||
of three pieces: | ||
For web apps, sound typing allows | ||
[dartdevc][] to generate cleaner, more compact JavaScript. |
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.
Should we combine "Better AOT" with "Cleaner JS" (essentially the latter is a special case of the former).
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 think I'll just delete the "cleaner JS" unless someone objects.
src/_guides/language/sound-dart.md
Outdated
|
||
### Example 1: From Map\<String, dynamic> to `var` | ||
|
||
Original definition: | ||
Explicit definition: |
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.
How about:
Explicitly type-annotated declaration:
or
Explicitly typed declaration:
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.
Or "Explicitly typed variable declaration". I think it's important to say that it's the type of the variable that's important here.
src/_guides/language/sound-dart.md
Outdated
|
||
<?code-excerpt "strong/lib/strong_analysis.dart (type-inference-1-orig)"?> | ||
{% prettify dart %} | ||
Map<String, dynamic> arguments = {'argA': 'hello', 'argB': 42}; | ||
{% endprettify %} | ||
|
||
New definition: | ||
Definition with var: |
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.
How about:
Implicitly typed definition using
var
:
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.
Implicitly typed variable declaration, using
var
:
src/_guides/language/sound-dart.md
Outdated
**[PENDING: That's unclear. Why isn't it `List<String>`? | ||
`foo` used to be `methodCall`, if that helps.]** | ||
{% endcomment %} | ||
|
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.
Maybe we can just drop this example because it is confusing more than anything else.
Why isn't it
List<String>
?
All we know is the foo
yields a value whose type can respond to operator []
methods and that the type of the argument to []
is String.
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 think it's confusing to use foo
before it's introduced. Even in the previous example.
I can't see what the example is trying to say, so either figure that out, or remove it entirely.
|
||
{:.fails-sa} | ||
<?code-excerpt "strong/lib/common_problems_analysis.dart (is-strong-mode-enabled)"?> | ||
{% prettify dart %} | ||
bool b = [0][0]; | ||
{% endprettify %} | ||
|
||
If you're using strong mode, you'll see the following issue from the analyzer: | ||
With type-safe Dart, the analyzer produces the following error: | ||
|
||
{:.console-output} | ||
<?code-excerpt "strong/analyzer-2-results.txt" retain="/'int' can't be .* 'bool'.*common_problems/"?> | ||
```nocode | ||
error • A value of type 'int' can't be assigned to a variable of type 'bool' • invalid_assignment | ||
``` |
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.
Here is an example of embedding an analyzer error.
I'm leery of changing the "sound" wording because @Sfshaza spent a lot of time getting the terminology right. On the other hand, the more consistent and correct and clear we are, the better. |
Adding people who reviewed the original page: @filiph @lrhn @leafpetersen |
I think overall this is a great improvement, thanks @kwalrath ! |
|
||
The simplest way to enable strong mode is to specify | ||
`strong-mode: true` in the analysis-options file: | ||
To find places in your code that could use explicit types, |
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 that case, implicit-casts
isn't relevant here. Those are perfectly fine and won't infer dynamic
.
So, what is the purpose of this section, now that it's no longer telling you how to enable strong mode?
The section promotes the part of the original that tells you how to enable more checks, to be a full section, but without the initial part, that feels out-of-place.
So, maybe a lead-in like:
You can enable more static checks on top of the Dart 2 type system,
to tell you where the type system would allow a down-cast by inserting a run-time check,
or where the type inference cannot find a useful type and defaults to inferringdynamic
.
{% endcomment %} | ||
|
||
You can use the flags together or separately. | ||
Both default to `true`. |
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.
Maybe:
Both default to
true
, which provides just the plain Dart 2 behavior with no extra warnings.
I want to emphasize that these flags provide warnings above and beyond what the language specifies, and it's perfectly reasonable to have programs with such warnings.
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 not really sure why we want to emphasize this?
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.
Good point. This document as all about flags to the analyzer, I think I was reading this as being in the context of describing strong mode.
On the other hand, I do find it extremely confusing that the flags default to true
, and that you turn the extra analyzer feature on by making them false
. I think that deserves some explanation, so I'd still like to say that true
means language-as-specified, and false
means to enable more analyzer restrictions. The flags would be better (more consistently) named avoid-infer-dynamic
and void-static-cast
.
(It made more sense when there was a strong-mode
flag, and these flags detracted from that flag in some sense, so you could read it as "+ strong mode, - dynamic casts". Now that strong mode is Dart, having analyzer flags detract from the language is less understandable).
The terms **sound** Dart and **type safe** Dart | ||
are often used interchangeably. | ||
You might also see the term **strong mode**. | ||
Strong mode was an opt-in Dart 1.x feature |
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.
Just "Dart 1 feature". Saying "1.x" seems curious when we should know exactly which "x" it applies to. We could say "Dart 1.19+" (number picked out of thing air).
src/_guides/language/sound-dart.md
Outdated
Not all tools have been updated to fully support | ||
the sound static type system. | ||
For example, dart2js and tools that depend on dart2js | ||
(such as DartPad) |
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.
That should change real soon now, right?
(Depending on our DartPad resources, it tends to be behind by quite a bit).
With strong mode enabled (in an implementation that has both the | ||
static and runtime checks), Dart is a statically typed language ensuring | ||
static and runtime checks), Dart 1.x is a statically typed language ensuring |
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.
Just "Dart 1".
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.
We've been saying 1.x consistently in the docs. If we change it here, we'll have to change it everywhere.
@mit-mit / @anders-sandholm / @timsneath, are we switching from Dart 1.x to Dart 1?
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.
To be honest, I don't think it's particularly worth the painstaking work to fix this up throughout the site. (I'd prefer to spend the time filling out some of the other doc gaps.) In a few months' time, I suspect we'll be gradually removing most references to Dart 1.x anyway, as Dart 2 becomes the only supported version of the language in common usage.
@@ -418,14 +363,14 @@ using `var`, the resulting map has type Map<String, Object>. | |||
|
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.
The "Map<String, Object>" just above doesn't need escapes or code quotes?
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.
Yikes, good catch.
src/_guides/language/sound-dart.md
Outdated
@@ -434,6 +379,8 @@ var arguments = foo['args']; | |||
|
|||
The resulting definition depends on the type of `foo` and |
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.
The type of
arguments
depends on ...
Not sure what "definition" covers (did it mean "declaration"?)
src/_guides/language/sound-dart.md
Outdated
**[PENDING: That's unclear. Why isn't it `List<String>`? | ||
`foo` used to be `methodCall`, if that helps.]** | ||
{% endcomment %} | ||
|
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 think it's confusing to use foo
before it's introduced. Even in the previous example.
I can't see what the example is trying to say, so either figure that out, or remove it entirely.
@@ -607,8 +542,8 @@ Do we have any known issues or bugs to list here? | |||
### The covariant keyword | |||
|
|||
Some (rarely used) coding patterns rely on tightening a type | |||
by overriding a parameter's type with a subtype, which is illegal in strong | |||
mode Dart. In this case, you can use the `covariant` keyword to | |||
by overriding a parameter's type with a subtype, which is illegal. |
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 prefer "invalid" to "illegal". Just personal peeve, but I don't consider us qualified to pass actual laws. We just get to define what is validly Dart.
by overriding a parameter's type with a subtype, which is illegal in strong | ||
mode Dart. In this case, you can use the `covariant` keyword to | ||
by overriding a parameter's type with a subtype, which is illegal. | ||
In this case, you can use the `covariant` keyword to | ||
tell the analyzer that you are doing this intentionally. | ||
This removes the static error and instead checks for an invalid | ||
parameter type at runtime. |
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.
"parameter type" -> "argument type"
(We generally use "parameter" about the formal parameters declared by a function and "argument" about actual parameters passed to the function by a function call).
Thanks @kwalrath ! |
I'm checking on whether I need to add strong-mode: true back to the analyzer docs.
Do we still require strong-mode: true to make the analyzer using Dart 2 semantics? | ||
Will these flags still appear under strong-mode in Dart 2.0? | ||
Should we mention related command-line flags | ||
(--no-implicit-casts, --no-implicit-dynamic)?** |
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.
For a short time, we will be living in a mixed world and I believe that strong mode will not be the default. Longer term, the flag should go away and, if we still want the "implicit-casts" and "implicit-dynamic" flags they should probably be moved out from under "strong-mode".
I don't know whether those flags will remain long term, but if they do, then we should probably document them.
Ready to go? |
@kwalrath is this "live"? I don't see the changes https://www.dartlang.org/guides/language/sound-dart ? |
It's in the master branch, which you can see at https://dartlang-org-staging-1.firebaseapp.com/guides/language/sound-dart. We'll switch www.dartlang.org to the master branch once Flutter switches to Dart 2. We've avoided updating the 1.x branch, except for crucial changes. To see more about the status (or get the URL for the Dart 2 version of dartlang.org), go to the meta issue, #637. |
Got it, thanks! |
Changes to the Dart 2 site to move away from strong mode, which no longer makes sense in Dart 2.
Staged at URLs like:
https://kw-www-dartlang-1.firebaseapp.com/guides/language/sound-dart
https://kw-www-dartlang-1.firebaseapp.com/guides/language/sound-problems
https://kw-www-dartlang-1.firebaseapp.com/dart-2 (interim change; more to come in another PR)
https://kw-www-dartlang-1.firebaseapp.com/guides/language/analysis-options
@bwilkerson Could you please review my changes to Customize Static Analysis? I made a lot of guesses.
/cc @anders-sandholm @mit-mit @mjohnsullivan
Contributes to #602
Fixes #676