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

Analyzer/Frontend inconsistency - type arguments after named constructor #34403

Closed
natebosch opened this issue Sep 7, 2018 · 28 comments

Comments

@natebosch
Copy link
Member

commented Sep 7, 2018

class Foo<T> {
  Foo.named();
}

void main() {
  Foo.named<String>(); // No error in analyzer or CFE
  new Foo.named<String>(); // Error in analyzer only
}

Dart 1 VM did not accept the second form.

It would be breaking to make the first form an error since it has no error in either place. The inconsistency between including or not including new is jarring.

@lrhn

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

It's an error, and should not be accepted by either tool.

The former syntax was accepted when we introduced generic methods, but since Foo.named is statically known to not be a generic method, the program must be rejected.

@matanlurey

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

I'm not sure how to label/triage this issue. I'm guessing CFE since its an error in the analyzer.

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

I'm guessing CFE since its an error in the analyzer.

They are both missing the error when you omit the new. We can split the issue if necessary.

@bwilkerson

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I'm guessing CFE since its an error in the analyzer.

The analyzer will not be implemented in terms of CFE in the 2.1 timeframe, so it needs to be implemented in both places. It would be good to have separate issues for the two areas.

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2018

Note that by fixing this we'll start to reject code that previously ran fine so it could break some projects.

I suspect we'll want to do that anyway, but can the language team please confirm that we want to move forward?

@askeksa-google

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Covered by #34159, #32972 and #34270.

@jensjoha

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

The implicit new case still doesn't work. Might be implicitly covered by 34159, but not explicitly covered by the other cases. Reopening.

@jensjoha jensjoha reopened this Sep 11, 2018

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

What's the status of this? We'll need to give users a heads up that there's a breaking bug fix coming down the pipe. Should we consider leaving this as a warning for a few dev releases?

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Are you asking about status in CFE, or Analyzer?
Analyzer started reporting this as an error.

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Looks like CFE doesn't fail on this yet so other than failing travis analyzer tests I don't think this should have a huge impact on flutter users yet.

This will currently be breaking for DDC users. If we make it a warning then DDC users won't be broken right on SDK update.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

@keertip have you found any third party packages affected by this?

@keertip

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Have not seen any third party packages affected by this. There is some internal code, DDC targets that fail due to this, not third party.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Ok, I see two options.

  • Roll this out as a breaking bug fix
  • Make it a warning. If and when we add generic constructors, then we can still say that using this syntax passes the type argument as the constructor parameter rather than the class parameter.

Either way, I think we need a little breathing room. @scheglov can you please demote this to a warning for now?

@scheglov

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

OK, this CL will make it a warning.

dart-bot pushed a commit that referenced this issue Sep 12, 2018
Downgrade WRONG_NUMBER_OF_TYPE_ARGUMENTS_CONSTRUCTOR to warning.
Per #34403 (comment)
request.

R=brianwilkerson@google.com

Change-Id: I9a2c319d2262aacf14bfc180d59d16db3f199a2a
Reviewed-on: https://dart-review.googlesource.com/74560
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@lrhn

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

I think it should be an error, and the sooner the better.
("A few dev-releases" might be a reasonable time-frame, although I would say that a dev-release is exactly where we should introduce the break, because users should not be running production code on dev-releases - we can then delay putting it into a stable release if we need to).

I doubt any code was "running fine" using this syntax since the code has no semantic meaning. Any code that gets away with writing this has no specified semantics. It is no more correct than applying type parameters to a non-generic function, and then calling it anyway and saying that the code "works".

Also, I very much want to introduce generic named constructors, and any leniency towards code that already uses that syntax will just delay breaking for that code until we actually start introducing a real meaning for it. That is not helping anyone.

@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

I would say that a dev-release is exactly where we should introduce the break, because users should not be running production code on dev-releases - we can then delay putting it into a stable release if we need to

I think that would be fine, I'm not sure if we typically keep those types of differences between dev and stable though - have we done it before?

I doubt any code was "running fine" using this syntax since the code has no semantic meaning.

It is treated by the VM and Dart2JS as if it does have the semantic meaning the user expected.

 class Foo<T> {
  Foo.named();
  void printType() => print(T);
}

void main() {
  Foo.named<String>().printType();
}

Prints "String" with VM and Dart2JS.

Also, I very much want to introduce generic named constructors, and any leniency towards code that already uses that syntax will just delay breaking for that code until we actually start introducing a real meaning for it. That is not helping anyone.

I agree that it's worth fixing to buy back the syntax for generic named constructors. Rushing in to breaking code that works today is what has me nervous.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

@danrubel This could also be an interesting use case for fixit tool.

@leafpetersen

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@natebosch do you have the time to shepherd the fix for this? I think we need to:

  • keep this as a warning for now
  • communicate on flutter-dev and dart-misc that this fix is coming
  • verify that third party packages used by flutter and google are warning clean
  • after a few dev releases with this warning in, promote to error (this needs coordination with the analyzer + CFE)
  • deal with any failures
@natebosch

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

The changes in the CFE are going to be the ones that are most impactful to the ecosystem. I think things would go the most smoothly if the rollout is managed by someone from the CFE team.

@vsmenon

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

This looks like an excellent example for an auto-fixup tool. @danrubel - wdyt?

@danrubel

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

It's on my list after the class to mixin fixup

@vsmenon

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

Thanks! What do you all think about using the auto-fixup tool to drive the rollout?

@askeksa-google askeksa-google referenced this issue Sep 28, 2018
7 of 7 tasks complete
@leafpetersen

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

@askeksa-google has a fix for this in the CFE. It sounds like we want to wait on this until we have a fixup tool? @danrubel what is the approximate time frame you see for having a tool for this? I'm somewhat inclined to push to get this fixed in time to get into the flutter 1.0 build, even if that means landing it in dev before we have the tool.

@danrubel

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

It's working in rough form to auto-fix class to mixin. For example:

dart pkg/analyzer_cli/bin/fix.dart /absolute/path/to/sdk/pkg/front_end/lib

I still need to hook it into the SDK.
I'll start looking at auto-fixing this issue tomorrow.

@danrubel

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Tasks:

  • Produce a unique warning for these situations
  • Add a quick fix/assist to move the type arguments to the correct location
  • Update dartfix to detect and correct this situation
  • Change warning to error - #34671
  • Integrate dartfix into the Dart SDK - deferred - see #34792

@danrubel danrubel self-assigned this Oct 8, 2018

@askeksa-google askeksa-google added this to the Dart2.1 milestone Oct 10, 2018

@askeksa-google

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

The CFE fix is ready at https://dart-review.googlesource.com/c/sdk/+/78080. The status file changes will probably conflict with the analyzer fix. But in the end, they should just eliminate all #34403 and #34671 entries.

@lrhn

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

For breaking bug fixes like this, is it an option to make it an error in the analyzer, but only a warning in the CFE (compilers) for a while. That would allow existing code to keep running, but not without being clearly marked as wrong by the analyzer.

@dgrove

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Decision: land this, whether the tool is ready or not.

@dart-bot dart-bot closed this in 5cbb640 Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.