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

Dart: do not export nested types defined in internal types #1606

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pwrobeldev
Copy link
Contributor

@pwrobeldev pwrobeldev commented Oct 24, 2024

In the case of Dart the nested types defined in types
annotated as @Internal were exported. This was invalid
behaviour -- such types shall not have been exported.

The root cause for that was usage of 'CommonGeneratorPredicates'
to check if type is internal. In the case of Dart there is no
type nesting and all types are outside. The common version of
predicate did not check if any of parent types uses @Internal.

This change:

  • moves Dart specific logic responsible for checking if LimeType
    is internal from DartVisibilityResolver.kt to DartGeneratorPredicates.kt
  • injects the predicate to DartVisibilityResolver via constructor in order
    to preserve the existing behaviour of checking if type is internal
  • fixes the way how DartGenerator.generateDart() selects types to export by
    using Dart specific predicate instead of the generic one

Note: this change adjusts smoke tests to show, that the internal types like
structs, classes and lambdas are no longer exported when their parent
type is @Internal.

@pwrobeldev
Copy link
Contributor Author

Notes for reviewers -- this PR is split into 2 commits:

  • the first one is big, but it contains only extension of smoke tests to show the invalid behavior of the generator -- the most important file is smoke.dart
  • the second one is small and it contains the actual fix for the problem

@pwrobeldev
Copy link
Contributor Author

Another important note -- even though the commit uses @Internal annotation it is not visible in the generated code. That's the desired behavior of Gluecodium. It is expected. Please see the linked comment for the explanation.

#1333 (comment)

@pwrobeldev pwrobeldev force-pushed the pwrobeldev/make-nested-struct-of-internal-type-also-internal branch from f6b598e to 2a1830d Compare October 24, 2024 12:59
The behaviour of generator for Dart is invalid in the case
of types that are nested in another type that is marked as
internal.

Such nested types of an internal type shall not be exported.
In order to show the invalid behaviour the smoke tests were
added.

In next commit Dart generator will be fixed and then the exports
visible in 'smoke.dart' file will be removed.

Signed-off-by: Patryk Wrobel <[email protected]>
In the case of Dart the nested types defined in types
annotated as internal were exported. This was invalid
behaviour -- such types shall not have been exported.

The root cause for that was usage of 'CommonGeneratorPredicates'
to check if type is internal. In the case of Dart there is no
type nesting and all types are outside. The common version of
predicate did not check if any of parent types are internal.

This change:
 - moves Dart specific logic responsible for checking if LimeType
   is internal from DartVisibilityResolver.kt to DartGeneratorPredicates.kt
 - injects the predicate to DartVisibilityResolver via constructor in order
   to preserve the existing behaviour of checking if type is internal
 - fixes the way how DartGenerator.generateDart() selects types to export by
   using Dart specific predicate instead of the generic one

Note: this change adjusts smoke tests to show, that the internal types like
      structs, classes and lambdas are no longer exported when their parent
      type is internal.

Signed-off-by: Patryk Wrobel <[email protected]>
@pwrobeldev pwrobeldev force-pushed the pwrobeldev/make-nested-struct-of-internal-type-also-internal branch from 2a1830d to 965271b Compare October 24, 2024 13:51
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.

1 participant