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

Class inheritance requires global namespace swapping #8

Closed
diamondburned opened this issue Jun 20, 2021 · 1 comment · Fixed by #13
Closed

Class inheritance requires global namespace swapping #8

diamondburned opened this issue Jun 20, 2021 · 1 comment · Fixed by #13
Labels
broken Changes that require refactoring

Comments

@diamondburned
Copy link
Owner

Preamble

Right now, the way class generation works, is that the implementation struct
only includes the parent type, which is usually the GObject itself. Interfaces
that it implements are generated as regular Go methods and conveniently utilizes
the class wrapper functions to reduce code.

Problem

The problem is, because some classes may implement interfaces from different
packages, and since the TypeTree routine naively uses the ResolveType API,
the returned methods of the searched interfaces do not contain the namespace.
These methods are then given to callableGenerator which then calls
TypeConverter and ResolveType, but since the namespace is not in the type
names, all methods will fail to resolve.

For example, if we're in package gdkpixbuf, and gdkpixbuf.Pixbuf implements
gio.Icon, then the searched-up Icon's methods will not contain gio. When
ResolveType sees this, it tries to restore the namespace by concatenating the
current namespace in, creating gdkpixbuf.Icon, which is false.

Below is the call tree that illustrates this problem:

  • classGenerator.Use
    • ifaceGenerator.UseMethods
      • updateMethods
        • newCallableGenerator (using ifaceGenerator's resolver)
        • callableGenerator.Use
          • NewTypeConverter
          • convert
            • ResolveType (using callableGenerator's resolver)

Solution

The best solution will involve several parts. First off, if the
callableGenerator were to take in a TypeResolver interface that would allow
the caller to override the current namespace for just this instance, then the
issue would then become that some code won't have the right namespace prefixes,
because the converter will mistaken the current namespace.

This means that the next solution to implement would be to somehow decouple just
type resolves away from the conversion. It might even make sense to give
ValueConverter two namespaces, the current one and the one to resolve with (or
the origin one).

Following that solution, ResolveType should then be completely decoupled away
from NamespaceGenerator at all. Instead, it should be its own function. We
could even make ValueConverter take in a callback ResolveType or a
TypeResolver interface that the caller can then use to wrap around and inject
its own namespace. Whatever it may be, the current ResolveType is only a
method because:

  1. Logging, which should be easily solved with (probably) option parameters (or
    a mini instance struct) that allow setting a LineLogger interface.
  2. Ignore searching, which is always part of Generator. This could be added
    into the TypeResolver interface.
  3. Type finding, which is also part of Generator and therefore can be in
    TypeResolver.
  4. ModPath, which formats the import module path into one that the caller
    wants, which can also be in TypeResolver.

From the list above, there are quite a few parts that rely on Generator that
it might be worth it to implement just one method that returns the Generator
instance to grab those common fields. It would also seem that a lot of these
abstractions should be their own packages, which would help decoupling a lot
more. Perhaps package girgen should only contain interfaces and the core
Generator.

A TypeResolver should not be file-dependent either. Ideally, it should be very
decoupled to the point of not having any side effects on anything at all
(including imports). This would make it a lot easier to reuse TypeResolver for
different purposes while easily allowing multiple files with versions and build
tags in the future.

@diamondburned
Copy link
Owner Author

GitHub is incredibly dumb with how they format issues. Blame them.

This was referenced Jun 20, 2021
@diamondburned diamondburned added the broken Changes that require refactoring label Jun 20, 2021
diamondburned added a commit that referenced this issue Jul 4, 2021
This commit closes #9 by adding the new girgen/file.go FileGenerator,
which is completely refactored from the ground up to be much smaller,
cleaner and side effects-free.

This commit closes #8 as well by introducing the SourceNamespace and
CurrentNamespace API inside typeconv.Converter, which allows the
converter to pick the right namespace for resolving and generating.
diamondburned added a commit that referenced this issue Jul 4, 2021
This commit closes #9 by adding the new girgen/file.go FileGenerator,
which is completely refactored from the ground up to be much smaller,
cleaner and side effects-free.

This commit closes #8 as well by introducing the SourceNamespace and
CurrentNamespace API inside typeconv.Converter, which allows the
converter to pick the right namespace for resolving and generating.
diamondburned added a commit that referenced this issue Jul 4, 2021
This commit closes #9 by adding the new girgen/file.go FileGenerator,
which is completely refactored from the ground up to be much smaller,
cleaner and side effects-free.

This commit closes #8 as well by introducing the SourceNamespace and
CurrentNamespace API inside typeconv.Converter, which allows the
converter to pick the right namespace for resolving and generating.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broken Changes that require refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant