LadybirdBrowser / ladybird

Truly independent web browser
https://ladybird.org
BSD 2-Clause "Simplified" License
21.63k stars 951 forks source link

LibIDL: Circular import issue #1855

Open beuss-git opened 2 weeks ago

beuss-git commented 2 weeks ago

Hey, I was wondering if the IDLParser should be able to handle circular imports.

The Parser::resolve_import() method in IDLParser.cpp does have a check for circular imports, but afaict it will never executed because when we do have a circular import, the top_level_resolved_imports() value will always contain the interface path as it is added there at the start of Parser::parse() which is recursively called in Parser::resolve_import().

This hasn't proved to be much of an issue though, and a lot of code seems to rely on circular imports such as CSSRule.idl <-> CSSStyleSheet.idl. I have attached a file containing a list of all the circular dependencies I could find here: circular_dependencies.txt (there are at least 51 cases).

The issue happens when we add mixins. And I first noticed it based on the work in #1715 because he had to separate out the mixin interface.

Currently the following happens when parsing an idl file:

  1. Add an "uninitialized" interface with the module path of the idl file to the top_level_resolved_imports() table
  2. For each import, resolve it:
    1. If it is in top_level_resolved_imports(), return it.
    2. Recursively parse this import by going to top step 1.
    3. Add this ("resolved") import list of imports of the idl file.
  3. Parse the interface or namespace, resolving the name, mixins and such
  4. Resolve mixins
  5. ...

Here is an example: A.idl

#import <B.idl>

interface A{ 
// B used here.
};

interface mixin C { };

B.idl

#import <A.idl>

interface B { };

B includes C;

We start by parsing A.idl, then the issue happens in step 4 when we are parsing B.idl in the recursive call to parse. B.idl at this point is happy with its imports, namely A.idl since it was added to the top level imoprts, but since A.idl hasn't actually been parsed, it reports that it has no mixins in its idl file. So B.idl will then complain that there exists no mixin named C.

If we just remove this line report_parsing_error(ByteString::formatted("Mixin '{}' was never defined", entry), filename, input, lexer.tell()); everything seems to still be working and the interfaces that include the mixin still get the mixin attributes. I guess this happens when we generate the bindings for each IDL individually.

I don't know the system well enough to tell what other implications this has.

Should anything be done with this?

Totto16 commented 5 days ago

Hi, the author of #1715 here. I also wondered why it's not done like this, since factoring out a mixin wasn't the best option, but it was the only one, that made everything work with the current IDL Parser, I also wonder, if there is any spec regarding IDL and the parsing order of it.

It works like I did it, and it doesn't seem to be a problem anywhere else, so I am not sure how urgent this is, or if it just is a one time problem 🤷🏼‍♂️