Skip to content

Enable strong-mode, test FutureOr. #1507

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

Closed
wants to merge 3 commits into from

Conversation

matanlurey
Copy link

Closes #1482.

This is a breaking change, and has been documented as such. Specifically, it looks like we now are using strong-mode inference, so many dynamic elements have become non-dynamic. That's probably a good thing?

(FutureOr looks great after this)

/cc @kevmoo @sethladd - is enabling strong-mode OK here?

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Sep 29, 2017
@kevmoo
Copy link
Member

kevmoo commented Sep 29, 2017

I'm a fan!

@matanlurey
Copy link
Author

I see two travis failures - one for Flutter and one for the core SDK after this.

dartdoc:stdout: [error] The list literal type 'List<dynamic>' isn't of expected type 'List<int>'. The list's type can be changed with an explicit generic type argument or by changing the element types. at /home/travis/build/dart-lang/dartdoc/doc/flutter/bin/cache/pkg/sky_engine/lib/typed_data/typed_data.dart, line 396.

A link to this SDK shows:

  static final Endianness HOST_ENDIAN =
      (new ByteData.view(new Uint16List.fromList([1]).buffer)).getInt8(0) == 1
          ? LITTLE_ENDIAN
          : BIG_ENDIAN;

... it looks kosher? I don't really understand this error.

Generation failed: 'package:dartdoc/src/model.dart': Failed assertion: line 3038 pos 12: '!name.isEmpty ||
        (this.element is TypeDefiningElement &&
            (this.element as TypeDefiningElement).type.name == "dynamic")': is not true.
dart:core                                                     _AssertionError._throwNew

I can fix this assertion to be fine with strong-mode, but I would like @jcollins-g's reaction first.

@kevmoo
Copy link
Member

kevmoo commented Sep 29, 2017 via email

@kevmoo
Copy link
Member

kevmoo commented Sep 29, 2017

FYI: I just tried running the code on master and got the same assert RE SDK.

@jcollins-g
Copy link
Contributor

The assert is fixed in #1506, however there are other problems in the flutter travis that seem to be related to pub that I'm currently debugging. I'll finish reviewing this once the build is fixed but the general principle looks good.

@matanlurey
Copy link
Author

Thanks @jcollins-g - not a priority if you're working on something else, just trying to help!

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've won my battle with the bots, this LGTM. Try merging with head and re-pushing the branch, bots should pass now.

@kevmoo
Copy link
Member

kevmoo commented Oct 2, 2017

woo hoo!

@matanlurey
Copy link
Author

@jcollins-g I still see:

dartdoc:stdout: [error] The list literal type 'List<dynamic>' isn't of expected type 'List<int>'. The list's type can be changed with an explicit generic type argument or by changing the element types. at /home/travis/build/dart-lang/dartdoc/doc/flutter/bin/cache/pkg/sky_engine/lib/typed_data/typed_data.dart, line 396.

on travis. Anything special I should do? Maybe I'm not synced?

@jcollins-g
Copy link
Contributor

There is likely an analysis option that is missing but required to properly analyze Flutter with strong mode enabled. Dartdoc doesn't obey analysis options files the same way analyzer does, so this change usually involves reading the affected code and any relevant analysis options files, and if possible, adding the option in the same place you just did.

@matanlurey
Copy link
Author

Sorry for the delay. I just re-merged against master, and tests pass locally.

Let's see if travis likes me 🙏

@matanlurey matanlurey force-pushed the strong-mode-future-or branch from 250fccf to 94d12a5 Compare October 11, 2017 18:49
@matanlurey
Copy link
Author

matanlurey commented Oct 11, 2017

😢 Flutter bot still doesn't like me.

It looks like it is:
https://github.com/dart-lang/sdk/blob/master/sdk/lib/typed_data/typed_data.dart#L396

I don't think it is an additional analysis option, it looks like it flags INVALID_CAST_LITERAL_LIST... unless this causes implicit-dynamic: false to trigger (https://github.com/flutter/engine/blob/master/analysis_options.yaml#L12).

@jcollins-g
Copy link
Contributor

I've got this mostly working in a different branch now, however there is still one little wart in Flutter -- we generate a new broken link with strong-mode on.

@jcollins-g
Copy link
Contributor

This probably should be blocked on #1561.

@jcollins-g jcollins-g added the status-blocked Blocked from making progress by another (referenced) issue label Dec 6, 2017
@matanlurey
Copy link
Author

Will be closed/replaced by #1611!

@matanlurey matanlurey closed this Feb 24, 2018
@matanlurey matanlurey deleted the strong-mode-future-or branch February 24, 2018 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded. status-blocked Blocked from making progress by another (referenced) issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants