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

[Constants-Phase1] Analyzer Issue for Constants Implementation #35080

Closed
14 of 15 tasks
JekCharlsonYu opened this issue Nov 6, 2018 · 13 comments
Closed
14 of 15 tasks

[Constants-Phase1] Analyzer Issue for Constants Implementation #35080

JekCharlsonYu opened this issue Nov 6, 2018 · 13 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Milestone

Comments

@JekCharlsonYu
Copy link
Contributor

JekCharlsonYu commented Nov 6, 2018

Request: As per implementation plan,

***Analyzer and CFE
**The analyzer and CFE implements support for the new constant and potentially constant expressions behind the experimental flag.

Phase 1 ETA: please see issue [tracker] (dart-lang/language#60)

Tasks:

@JekCharlsonYu JekCharlsonYu added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Nov 6, 2018
@JekCharlsonYu JekCharlsonYu added this to the Dart2.2 milestone Nov 6, 2018
@stereotype441 stereotype441 added the type-enhancement A request for a change that isn't a bug label Nov 7, 2018
@bwilkerson bwilkerson self-assigned this Nov 8, 2018
@bwilkerson
Copy link
Member

@lrhn @mit-mit

I started looking at what it would take to implement this experiment, and I found that some of the work is already done. For example, the && and || operators have been lazy in the analyzer since 1/30/18 (https://dart-review.googlesource.com/37540).

We could potentially put the current implementation behind the flag and then revert the change to make them eagerly evaluate their arguments, but I'm concerned that that would break existing users.

Are we certain that all of these changes are really different than the 2.1 behavior?

How would you like me to handle this situation?

@bwilkerson
Copy link
Member

The rules for equality ('==' and '!=') were updated on 8/23/18 (https://dart-review.googlesource.com/71340), so they are also already in 2.1.

@lrhn
Copy link
Member

lrhn commented Nov 13, 2018

It seems that == and != works everywhere (analyzer, dart2js, VM),
but && and || only works in the analyzer and VM, they fail in dart2js.
In the VM, anything goes, really, even things that were never valid. It's probably a case of CFE missing errors, where dart2js has its own constant system catching the error, and running the old rules.

It does mean that there might be code out there using these features, passing the analyzer and running on the VM, so we can't retract the already working functionality.
So, ==, !=, && and || functionality should keep working without the experimental flag in the analyzer and VM (CFE). Dart2js is free to keep it not working without the experimental flag, or just follow the CFE.

@bwilkerson
Copy link
Member

Thanks!

@sigmundch (For the dart2js reference in the previous comment.)

@stereotype441
Copy link
Member

Note that when people start using the new cast and type test operators, they are likely to run into trouble due to #33441, because the analyzer doesn't always assign correct types to constants that are maps or lists. Accordingly, I'm adding a bullet item to fix #33441 before we consider the implementation complete.

@MichaelRFairhurst
Copy link
Contributor

MichaelRFairhurst commented Dec 18, 2018

#33441 has been closed, the real issue behind it is #35441, which will not affect this issue to nearly the same degree.

I'm still hoping to solve #35441 this week, but its not in my opinion a blocker.

@stereotype441
Copy link
Member

We've now completed all the implementation work we know about, but the co19 tests aren't passing, so we're investigating that.

@stereotype441
Copy link
Member

Note: there are some new co19 failures (co19_2/LanguageFeatures/Constant_update2018/...) on bot analyzer-linux-release, analyzer-mac-release, and analyzer-win-release as of today.

@mit-mit mit-mit modified the milestones: Dart2.2, Dart2.2.1 Feb 26, 2019
@stereotype441
Copy link
Member

I'm placing this in the Dart2.3 milestone because @kmillikin and I believe some work may be necessary in order to make the analyzer and the front end behave consistently in Dart 2.3. I'll be meeting with him tomorrow to clarify the details and I'll update this bug after I do so.

@stereotype441
Copy link
Member

Ok, I've just finished meeting with @kmillikin. The FE is planning to ship the full functionality of this feature in Dart 2.3, so the analyzer team will plan to do so as well.

@stereotype441 stereotype441 added the P2 A bug or feature request we're likely to work on label Mar 22, 2019
@dgrove
Copy link
Contributor

dgrove commented Apr 2, 2019

Is there more remaining here for 2.3? ( #36296 is no longer in the milestone, but is in the meta-issue list above)

@bwilkerson
Copy link
Member

Not all of the SDK constraint checks have been implemented, but I'm not sure that the remaining ones are worth worrying about. I don't know how @stereotype441 feels about it, but I'd be fine with closing this.

@stereotype441 stereotype441 modified the milestones: Dart 2.3, Dart 2.4 Apr 4, 2019
@stereotype441
Copy link
Member

Closing since the only remaining work is covered under #36296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

7 participants