How to work with For?

[Xazax-hun]

In C++, all parts of For are optional, but in IPR, the getter methods of For always wants to return something. If we do not fill one of the fields we will get an assertion fail. Previously, we used the Empty_stmt to always fill those fields. But bd6467e removed it. How should IPR represent a for statement with missing components?

[GabrielDosReis]

The missing expressions of the For should be thought as instances of ipr::Phantom.
ipr::Empty_stmt shouldn't have been used there, regardless.

[Xazax-hun]

ipr::Phantom works for For. However, we also used Empty_stmt with a special type to handle missing/unsupported nodes. E.g., if we wanted to convert a node from Clang AST to IPR but we either did not implement it in IPR yet, or IPR did not have an equivalent at the time, instead of crashing, we ended up creating an Empty_stmt placeholder. Do you have any recommendation what to use for this purpose with Empty_stmt removed? I cannot use a Phantom because that is not a statement.

[GabrielDosReis]

ipr::Phantom works for For.

As expected; ipr::Empty_stmt should not have been used in those situations.

However, we also used Empty_stmt with a special type to handle missing/unsupported nodes.

That sounds like a serious misuse of ipr::Empty_stmt.
That node was nothing more that an (in hindsight) ill-advised premature optimization of indirection. The only place it should have been used was as representation of an empty expression statement or a null statement. Given that description, I would say the removal didn't happen early enough.

Do you have any recommendation what to use for this purpose with Empty_stmt removed?

Well, the only semantics of ipr::Empty_stmt was that of an ipr::Expr_stmt where expr() is a ipr::Phantom.

For the purpose of not-yet-implemented node, if the use of exceptions is not appropriate, I would suggest you want to mark each of those instances with specific nodes; for example, you could introduce your own dedicated implementation nodes, or you could create dedicated ip::Symbol nodes for each of those cases so you can unambiguously tell which node is missing.

[Xazax-hun]

I would suggest you want to mark each of those instances with specific nodes; for example, you could introduce your own dedicated implementation nodes

Is IPR extensible?

If I try to add a new node like struct MyStmt : impl::Stmt { ... };, how can I add this to the visitors?

Node is implemented like this:

   template<class T>
   struct Node : T, util::immotile {
      using Interface = T;
      void accept(ipr::Visitor& v) const final { v.visit(*this); }
   };

The problem is, even if I add struct MyVisitor : ipr::Visitor { ... };, MyVisitor::visit(const MyStmt*) will not be part of the overload set in the body of MyStmt::accept.

Do I miss something?

[GabrielDosReis]

[Gabor]

If I try to add a new node like struct MyStmt : impl::Stmt { ... };,

[GDR]

[...] you could introduce your own dedicated implementation nodes [...]

so I would have expected the code to read

struct MyStmt : ipr::Stmt { ... };

how can I add this to the visitors?

The visitor operates on the interface classes, so any class (including your own) that derives from those interface classes will be visited. Is that not happening? Why?

[Xazax-hun]

Is that not happening? Why?

Because this is how overload resolution works in C++?

Here is a minimalized example, but let me know if I am missing something:

#include <iostream>
struct Visitor;
struct Base {
    void accept(Visitor*);
};
struct Derived : Base {};
struct Visitor
{
    void visit(Base*) { std::cout << "Base\n"; }
};
struct DerivedVisitor : Visitor
{
    void visit(Derived*) { std::cout << "Derived\n";  }
};
void Base::accept(Visitor* v) { v->visit(this); }
int main()
{
    DerivedVisitor dv;
    Derived d;
    d.accept(&dv);
}

[Xazax-hun]

Note that, with the solution above, none of the convenience features in IPR would work with the new node. For example, ipr::util::view would not be able to do the conversion to the newly added node. These extended nodes feel like second class citizens.

[GabrielDosReis]

I am not understanding the purpose of this line and how it captures or fits in the issue at hand

virtual void visit(DerivedInterface*) { std::cout << "Derived\n"; }

Could you elaborate?

[Xazax-hun]

I am not understanding the purpose of this line and how it captures or fits in the issue at hand

When I add a new node, I want to be able to work with that new node. I want to be able to visit it as a most derived type so I can extend the printer to print the new node. I want to be able to use something like ipr::util::view so I can do down casts conveniently and safely. I could always litter the current visitors with dynamic casts, but that does not feel like the right solution.

[GabrielDosReis]

[Gabor]

Note that, with the solution above, none of the convenience features in IPR would work with the new node. For example, ipr::util::view would not be able to do the conversion to the newly added node. These extended nodes feel like second class citizens.

1. Slow down; you're moving too fast and there many confusing aspects to your reduced example, and it is not clear if they are capturing what I am saying
2. I don't know what you mean by "second class citizen", but "not yet implemented" node isn't an IPR node.
3. Let's take a step back to the original reply and make sure we understand each of them.
4. Let's assume each of us understand C++, so "why" questions are not about explaining C++ but invitation to clarify a statement

[Xazax-hun]

Ok. Let's concentrate on one use case. When I add a Not_implemented node, I want that node to be printed in the serialization, so I can write regression tests to verify the IPR has the structure I expect. The printer is a visitor. It needs a visit method for Not_imlemented node (like visit(const Not_implemented*)), otherwise I am forced to add dynamic_casts to visit(const *Stmt). It would be even better if ipt::util::view<Not_implemented>(node) worked, so the added node would behave the same way as if it was the part of IPR.

[GabrielDosReis]

Ok. Let's concentrate on one use case. When I add a Not_implemented node, I want that node to be printed in the serialization, so I can write regression tests to verify the IPR has the structure I expect.

When you add that Not_implemented node, the first question is: is it an implementation node or it is an interface node?

[Xazax-hun]

From my point of view, it does not matter. Whichever can provide the behavior would be fine. But based on the discussion so far, I'm better off with an Interface node.

[GabrielDosReis]

From my point of view, it does not matter

From IPR perspective. It does matter. So, we need an answer.

But based on the discussion so far, I'm better off with an Interface node.

I am not clear about the logical steps that lead to that conclusion. Could you unpack that for me so I get a better understanding?

[Xazax-hun]

I am not clear about the logical steps that lead to that conclusion. Could you unpack that for me so I get a better understanding?

It followed from one of your earlier statements:

The visitor operates on the interface classes,

But so far I do not have a good understanding why this decision is consequential for my problem.

[GabrielDosReis]

It followed from one of your earlier statements:

How?

But so far I do not have a good understanding why this decision is consequential for my problem.

Then help me understand better your problem, so I can help you understand why that determination matters.

[Xazax-hun]

As far as I understand, if I add Not_implemented as an interface, its accept method will be overwritten by impl::Node. If I add it as an implementation, impl::Node::accept is final, so I still cannot change its behavior. As far as I can see, the same accept implementation will be invoked in both cases, so I do not see how this would change anything about my problem. And that implementation is using ipr::Visitor, not MyVisitor. Or I can skip impl::Node alltogether. But I still need dynamic casts and some ipr facilities like view would still not work.

[GabrielDosReis]

Gabor -

You are still describing an implementation tactic without describing the real problem that you're trying to solve. In doing so, you seem to be hitting some walls that aren't necessarily there if we started from a shared understanding of what the real problem is.

Can you start over again by describing the real problem, and then we work from there to possible solutions?

[Xazax-hun]

Can you start over again by describing the real problem, and then we work from there to possible solutions?

I am not sure what you want. I described the high level goal earlier: When I add a Not_implemented node, I want that node to be printed in the serialization, so I can write regression tests to verify the IPR has the structure I expect.

I also described the low level problem. Specifically, MyVisitor::visit(const Not_implemented*) is not being called without doing something hacky.

At this point I am not sure what else can I describe about the problem.

[d-winsor]

To provide more input into the problem. Tooling is currently built on top of the IPR interface with no connection to any implementation details so that this tooling can operate no matter what the underlying implementation is. I believe this is the key point that is missing in this discussion.

Currently we can not construct a usable IPR tree from a extensive compiler such as Clang without having a method to produce dummy nodes for C++ constructs that do not yet exist or have limitation in the state of the IPR today. The implementation for Clang used Empty_stmt to represent non-existent statements, As_type(Literal("unsupported_type_)) to represent unsupported types, and alias("<unsupported>") for Decls.

Obviously this is a hack, these Unsupported nodes aren't first class citizens in the IPR but we need some way to materialize them so that we can progress with producing and using an IPR graph while the IPR specification is being defined and improved. To resolve this we could:

1. Add a new node to the IPR interface which lets us handle this scenario as if it were another node in the IPR. This would have a visitor overload be usable in other utility functions such as util::view
2. Use the non-leaf nodes (ipr::Expr, ipr::Stmt, ipr::Type) and create a new implementation node of those types with reasonable values for the different functions (Type(), Name(), etc.)
3. Crash on creation of Trees with unsupported nodes which will greatly limit the ability to test against real world code.

Option 2 appears to be what @GabrielDosReis is suggesting and that could be implemented with something like the example below. If we go down this route we would be creating what I would perceive as an ill defined IPR tree. These non-leaf nodes are clearly defined as abstract nodes and as a user looking at the interface I would have the expectation to never see an instance of these classes.

namespace ipr::impl {
struct Not_impl_stmt : Node<ipr::Stmt> {
    category = Category_code::Unknown
}}

The idea of being able to create new Node not explicitly defined in the Interface and how that interacts with existing utilities that tools will be using such as Visitors or util::view (dynamic_cast) is an interesting discussion to have. It brings up the idea of extensibitly and if or how the IPR can be built upon to meet the needs of different non-standard compiler extensions or different tooling. This is not really the right place to have that discussion but it is something important that we should explore or at least explicitly define a stance on.

I suggest we go with option 2 but I am open to more thoughts. This will involve explicitly baking in the understanding that abstract Nodes can have implementations or child Nodes not defined in the interface so this is just replacing one hack with another one but at least we can make progress.

[Xazax-hun]

Thanks for summarizing! In the discussions earlier in this thread I wanted to push for option 1, but wanted to start with the description of the problem instead of offering a solution upfront. I think having ipr interface nodes that are visited by ipr::Visitor, e.g., something like ipr::ExtensionStmt would make our job very convenient adding new nodes. We could add our own visitor for the nodes derived from ExtensionStmt, and our override of ipr::Visitor::visit(const ExtensionStmt*) would hand the work over to our ExtensionNodeVisitor.

Option 2 is also viable, but I think that would result in less elegant code in the Clang->ipr tool.

Option 3 does not feel a viable alternative.

[GabrielDosReis]

@d-winsor

Thanks for this thoughtful write up. Very useful.

As part of understanding the problem is, I have a question about this:

Currently we can not construct a usable IPR tree from a extensive compiler such as Clang without having a method to produce dummy nodes for C++ constructs that do not yet exist or have limitation in the state of the IPR today. The implementation for Clang used Empty_stmt to represent non-existent statements, As_type(Literal("unsupported_type_)) to represent unsupported types, and alias("") for Decls.

Would it make sense to work off those missing (equivalent IPR) types so we don't discovered down the line that there are other kinds of surprising uses that lead to the original question?