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

Specify implicit construction #3040

Merged
merged 7 commits into from
Aug 24, 2023
Merged

Specify implicit construction #3040

merged 7 commits into from
Aug 24, 2023

Conversation

eernstg
Copy link
Member

@eernstg eernstg commented May 4, 2023

This PR adds a proposal to the working directory about implicit constructors, provided as members of static extensions.

@eernstg eernstg requested a review from leafpetersen May 12, 2023 18:43
@eernstg
Copy link
Member Author

eernstg commented May 12, 2023

The proposal should be complete enough to enable some discussions at this point.

@sigmundch
Copy link
Member

Thanks so much for putting this together @eernstg. There are many elements of this proposal I really like. I agree it's important to limit the scope of the implicit constructors. I think requiring a show may actually be an interesting requirement, or I'd potentially go even further and require that developers state what is the implicit conversion itself (showing an extension may not really hint to someone reading the code what's the on-class or what's the constructor that is in use).

For example:

import 'foo.dart'; 
   show E implicit constructor Distance Function(int);
...

I know my suggestion is very verbose, but just sharing the concept here 😄

We could also go further and limit conversions to different scope levels (library, class, or even method):

@ImplicitConversions([Distance Function(int)]) // conversion allowed in this library
import '...';

...
@ImplicitConversions([Distance Function(double)]) // conversion only allowed in this class
class A {
  ...
  @ImplicitConversions([Distance Function(String)]) // conversion only allowed in this method
  m1() {
  }
}

Thoughts?

On a separate note, this last week I came across interesting cases with == and != operations. Technically == operators are a potential assignment point, but, because == is defined as taking an Object argument we wouldn't be able to recognize it as a place to use an implicit conversion. Here is a concrete example:

int height = 0;
if (height == api.height /*returns JSNumber */) {
 ...
}

What are your thoughts on possible ways in which == could also be considered a trigger for conversions?

@eernstg
Copy link
Member Author

eernstg commented May 22, 2023

@sigmundch wrote:

require that developers state what is the implicit conversion itself

Interesting idea! We could allow implicit conversions to be enabled explicitly, and then have a lint catching the cases where it isn't used (a la "you're using this-and-that implicit constructor to convert [a specific expression], but that constructor hasn't been explicitly enabled").

// Library 'client.dart'.
import 'js_interop_static_extensions.dart' enable DoubleJsExtensions.fromDouble;

JSNumber jsNumber = ...;
double d = jsNumber; // Uses implicit conversion.

We could also rely on the function type (here: double Function(JSNumber)), but I think it's a safer bet to enable each implicit constructor by name: There could be multiple implicit constructors with the same function type, or there could be subtypes or supertypes of that function type. If the name is mentioned explicitly then it should be rather easy to find the corresponding function type, at least in an IDE, by hovering or by going-to-declaration.

This would again be a mechanism that we can leave out at first, and it could then be introduced later, and developers could make the choice about whether/when/where they'd enable the lint. For instance, there could be a library where certain conversions are used all the time, and enable Some, constructors will be a significant convenience (or some developers might even prefer // ignore_for_file: implicit_conversion_not_explicitly_enabled), and there could be a different library where those conversions are a rare exception, and the maintainers prefer to write the constructor invocation explicitly.

What are your thoughts on possible ways in which == could also be considered a trigger for conversions?

[Edit: changed my response here]

I believe the position as the right operand of the == operator should not be subject to implicit conversion, unless it calls an operator == whose signature is statically known to be non-standard: In the standard case where the parameter type is Object, the context type for that operand is dynamic (according to a small experiment I just performed ;-), and it should in any case be a top type, because the semantics of the evaluation of e1 == e2 handles null as well as the parameter type Object.

If we do have an override like bool operator ==(covariant SomeType other) => ...; then the context type would presumably be SomeType?, cf. #3098.

So we wouldn't have to do anything special about ==: Conversion to a top type will never occur implicitly, because every type is assignable to a top type. For the rare covariant SomeType case, we would presumably use the context type as stated, and conversions would take place just like any other location in the code which is an assignment position.

Copy link
Member Author

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Review response (in this particular case I actually did not change the PR, but there's a discussion in #3095).

@eernstg eernstg force-pushed the spec_implicit_conversion_may23 branch from b39c84a to b168793 Compare August 22, 2023 12:37
@eernstg
Copy link
Member Author

eernstg commented Aug 24, 2023

Following the approach used with the primary constructor proposal, I'll land this document in 'working' at this time.

@eernstg eernstg merged commit ccb70ae into main Aug 24, 2023
@eernstg eernstg deleted the spec_implicit_conversion_may23 branch August 24, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants