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.

Sign up for GitHub

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

Jump to bottom

[swift2objc] Support wrapper classes for primitives #1984

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[swift2objc] Support wrapper classes for primitives #1984

wants to merge 3 commits into from

Conversation

AmrAhmed119
Copy link
Contributor

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@AmrAhmed119
Copy link
Contributor Author

atm I have declared the primitiveWrapperClasses as a global variable instead of passing it to most of the functions in the transform module, let me know what is the best for this situation, Thanks.

@liamappelbe
Copy link
Contributor

liamappelbe commented Feb 12, 2025 •
edited
Loading

atm I have declared the primitiveWrapperClasses as a global variable instead of passing it to most of the functions in the transform module, let me know what is the best for this situation, Thanks.

That's fine. Nothing wrong with immutable global variables. As long as you're not adding more elements to this map at runtime, it ok for it to be global.

Add me as a reviewer when this PR is ready for review.

@github-actions GitHub Actions
Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️ 
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@AmrAhmed119
Copy link
Contributor Author

That's fine. Nothing wrong with immutable global variables. As long as you're not adding more elements to this map at runtime,

I think now I'll have to change it, but can you review the overall approach first.

Add me as a reviewer when this PR is ready for review.

Yea it's ready, but I think I don't have permissions to add reviewers for PRs.

@liamappelbe liamappelbe self-requested a review February 13, 2025 02:10
@github-actions GitHub Actions
Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:ffi 2.1.4 ready to publish ffi-v2.1.4
package:ffigen 17.0.0-wip WIP (no publish necessary)
package:jni 0.14.0 already published at pub.dev
package:jnigen 0.14.1-wip WIP (no publish necessary)
package:objective_c 5.0.0 already published at pub.dev
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@coveralls
Copy link

coveralls commented Feb 16, 2025 •
edited
Loading

Coverage Status

coverage: 85.261%. first build
when pulling 46b1921 on AmrAhmed119:throw-primitive-types
into 4746dc4 on dart-lang:main.

liamappelbe
liamappelbe reviewed Feb 16, 2025
View reviewed changes
pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart Outdated
import '../../transformer/_core/utils.dart';
import '../transform.dart';

final primitiveWrappers = <ReferredType, ReferredType>{
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not actually using this as a map. It's only used by getPrimitiveWrapper, and that iterates through it. So it can be a list instead. In fact, it can be an unmodifiable list.

final primitiveWrappers = List.unmodifiable(<(ReferredType, ReferredType)>[

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When declaring an unmodifiable list as above, the type inference fail, causing the list to be assigned a dynamic type. This lead to analyzer issues.

Also when I declare the type explicitly it causes analyzer issues.

So i create it as mutable list for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if you try List<(ReferredType, ReferredType)>.unmodifiable(<(ReferredType, ReferredType)>[? Or List<(ReferredType, ReferredType)>.unmodifiable([?

pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart Outdated Show resolved Hide resolved
pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart Outdated Show resolved Hide resolved
pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart Outdated Show resolved Hide resolved
pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart Outdated Show resolved Hide resolved
liamappelbe
liamappelbe reviewed Feb 18, 2025
View reviewed changes
pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart
}

ReferredType? getPrimitiveWrapper(DeclaredType other) {
final primitiveWrappers = <(ReferredType, ReferredType)>[
Copy link
Contributor

Choose a reason for hiding this comment

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

Make primitiveWrappers a private global. If you declare it in here it'll be reconstructed every time.

pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart
return (wrapper, true);
}

ReferredType? getPrimitiveWrapper(DeclaredType other) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this private: _getPrimitiveWrapper. I suggest a refactor in maybeWrapValue that will remove the need for calling getPrimitiveWrapper in transform_function.dart.

pkgs/swift2objc/lib/src/transformer/_core/primitive_wrappers.dart
// Support Optional primitives as return Type
// TODO(https://github.com/dart-lang/native/issues/1743)

(ReferredType, bool) getWrapperIfNeeded(
Copy link
Contributor

Choose a reason for hiding this comment

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

Google style is to call functions that may or may not do a thing maybeFoo instead of fooIfNeeded. So call this function maybeGetPrimitiveWrapper.

pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart
@@ -90,10 +91,18 @@ MethodDeclaration _transformFunction(
transformationMap,
);

final shouldWrapPrimitives = originalFunction.throws &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this variable just equal to originalFunction.throws. The other two conditions will be implicitly checked inside maybeWrapValue, if you do the refactor I suggest in there.

pkgs/swift2objc/lib/src/transformer/_core/utils.dart
(String value, ReferredType type) maybeWrapValue(ReferredType type,
String value, UniqueNamer globalNamer, TransformationMap transformationMap,
{bool shouldWrapPrimitives = false}) {
if (shouldWrapPrimitives) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor that will simplify other stuff:

  final (wrappedPrimitiveType, returnsWrappedPrimitive) =
      getWrapperIfNeeded(type, shouldWrapPrimitives, transformationMap);
  if (returnsWrappedPrimitive) {
    return ('${(wrappedPrimitiveType as DeclaredType).name}($value)', wrappedPrimitiveType);
  }

pkgs/swift2objc/lib/src/transformer/transformers/transform_function.dart
@@ -174,6 +185,7 @@ List<String> _generateStatements(
resultName,
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to call maybeWrapValue again. Reuse the result of the earlier call.

pkgs/swift2objc/lib/src/transformer/transformers/transform_variable.dart
if (originalVariable.throws || originalVariable.async) {
final prefix = [
if (originalVariable.throws) 'try',
if (originalVariable.async) 'await'
].join(' ');

final (type, _) = getWrapperIfNeeded(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. This is a bit suspicious. Should you be calling maybeWrapValue here, and using the wrapping code to construct the return statement on line 98?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamappelbe liamappelbe liamappelbe left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
package:swift2objc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[swift2objc] Support throws on methods returning primitive types
3 participants
@AmrAhmed119 @liamappelbe @coveralls