dmd icon indicating copy to clipboard operation
dmd copied to clipboard

Fix Issue 15749 - Implement with expressions

Open maxhaton opened this issue 2 years ago • 19 comments

I've definitely missed some things but this is enough to be reviewed (i.e. non-draft).

I haven't decided what to do with the union usage yet. Hypothetically one could use the same symbol table thing the WithStatements use and avoid any such thing entirely, but this seemed very ugly i.e. dmd already conflates a bunch of things behind it's back as is so making Scope granular on the level of expressions is a bit ridiculous in my mind.

maxhaton avatar Mar 07 '22 07:03 maxhaton

Thanks for your pull request, @maxhaton!

Bugzilla references

Auto-close Bugzilla Severity Description
15749 enhancement allow `with` on an expression

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13776"

dlang-bot avatar Mar 07 '22 07:03 dlang-bot

I assume I've broken one or part of the visitors

maxhaton avatar Mar 07 '22 09:03 maxhaton

I assume I've broken one or part of the visitors

The vtable layout has changed, so yes.

ibuclaw avatar Mar 07 '22 12:03 ibuclaw

ccing @pbackus @CyberShadow since they have proposed library implementations for bind.

RazvanN7 avatar Mar 07 '22 12:03 RazvanN7

with is already controversial because of the hiding issue, I think anything that extends it would need a larger discussion (i.e. a DIP).

CyberShadow avatar Mar 07 '22 12:03 CyberShadow

The main difference between this and bind (or a with statement wrapped in a lambda) is that a with expression does not introduce a new scope.

pbackus avatar Mar 07 '22 13:03 pbackus

I've wanted this for a long time, but I think this should go through a DIP proposal first.

iK4tsu avatar Mar 07 '22 13:03 iK4tsu

@iK4tsu Why isn't the library solution a good alternative?

RazvanN7 avatar Mar 07 '22 15:03 RazvanN7

I didn't know about the library proposals. The bind implementation looks really good actually. One thing it does not provide is access to members through the type.

struct Foo { ... }
...
with(Foo) ...

However, this is really nitpicking and I don't have right now a concrete example where it could be crucially useful.

The other thing is that it kinda completes the with feature we have right now. It just feels weird and unintuitive that we can't use with with expressions, and it gives an incomplete feature kind of vibe to me. This is not really a strong point to support the change, just an opinion. Someone new to the language finds about with, but then sees that it is really restrictive and its use cases are too few to even bother using it, then he learns that the useful stuff that should be done with with is actually a library implementation. This is not a rant btw. I just think that the library implementation would look a bit "out of place" when we have with and with would look even more useless.

Having said that, I would totally use the library solution if that's the decision you're heading for :smile:

iK4tsu avatar Mar 07 '22 17:03 iK4tsu

@RazvanN7 as mentioned above you can implement a terrible approximation to this as a library, yes, but it doesn't actually generalize with to expressions so the comparison is pointless.

maxhaton avatar Mar 08 '22 01:03 maxhaton

I agree with extending with to an expression. A library solution would be a workaround that is slower and can lead to more unclean code. If we have with, why not just use it?

ljmf00 avatar Mar 09 '22 23:03 ljmf00

@maxhaton you also forgot to add the new Expression node to astbase.d.

ljmf00 avatar Mar 09 '22 23:03 ljmf00

@ljmf00 removing the union requires making my visitor much more complicated as I now have to walk everything myself versus just hooking where the identifiers appear

maxhaton avatar Mar 09 '22 23:03 maxhaton

@ljmf00 removing the union requires making my visitor much more complicated as I now have to walk everything myself versus just hooking where the identifiers appear

I understand that, but I don't see the beneficial usage of a union in public fields on such a high-level interface, either in terms of performance or memory usage. AFAIU, I would say it is just harder to implement but way safer if someone decides to use this field in the future. I don't have any suggestions other than what you suggested above, but if anyone comes up with an idea of making it safer, would be cool.

ljmf00 avatar Mar 10 '22 14:03 ljmf00

@ljmf00 removing the union requires making my visitor much more complicated as I now have to walk everything myself versus just hooking where the identifiers appear

Does it really though? Outside of unary and binary nodes, what else is there to have a visitor for?

ibuclaw avatar Mar 10 '22 19:03 ibuclaw

@ljmf00 removing the union requires making my visitor much more complicated as I now have to walk everything myself versus just hooking where the identifiers appear

Does it really though? Outside of unary and binary nodes, what else is there to have a visitor for?

Just because something inherits from a UnaExp doesn't mean it only has one member, e.g. to walk things properly you need to handle all the annoying stuff.

I can make the union safe I just haven't worked out the best way of doing it yet. A property would probably do the job.

maxhaton avatar Mar 10 '22 23:03 maxhaton

Does it really though? Outside of unary and binary nodes, what else is there to have a visitor for?

Just because something inherits from a UnaExp doesn't mean it only has one member, e.g. to walk things properly you need to handle all the annoying stuff.

There are examples of rewrite visitors in the front-end so you're not going at it alone. Secondly, WithStatements manage just fine without this union.

ibuclaw avatar Mar 11 '22 09:03 ibuclaw

Does it really though? Outside of unary and binary nodes, what else is there to have a visitor for?

Just because something inherits from a UnaExp doesn't mean it only has one member, e.g. to walk things properly you need to handle all the annoying stuff.

There are examples of rewrite visitors in the front-end so you're not going at it alone. Secondly, WithStatements manage just fine without this union.

Take a look at how with statements are implemented... It uses a completely different system: the symbol table is effectively augmented to search the with-ed symbol first. This does not scale to expression very well if at all because it relies on the Scope structure which hypothetically could be bludgeoned into being granular at the level of an expression but would be extremely ugly and probably would break nested expressions etc.

maxhaton avatar Mar 11 '22 09:03 maxhaton

So, is there any future for this?

RazvanN7 avatar Apr 27 '22 09:04 RazvanN7