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

fix issue 3546 - Support for alias of individual array and aggregate elements #11273

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

Conversation

ghost
Copy link

@ghost ghost commented Jun 15, 2020

An alias allows to reference a variable. When used this alias to a var is then turned into a VarExp.
The idea is why not allowing an extra index, as long as the index is for a variable and that it is a compile-time constant ?

@ntrel
Copy link
Contributor

ntrel commented Jun 15, 2020

If you rename the title to 'Fix Issue'... then the bot should automatically link this pull to bugzilla.

@MoonlightSentinel
Copy link
Contributor

The bot doesn't pick up draft PR's IIRC.

@wilzbach
Copy link
Member

The bot doesn't pick up draft PR's IIRC.

Yup and it only looks at commit messages, because the changelog will be assembled from commit messages.

@ntrel
Copy link
Contributor

ntrel commented Jun 15, 2020

Oh sorry. Seems like the bot linking to bugzilla even for drafts would be useful.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

This raises the question why it's limited to index expressions and e.g. disallows function calls.

Fun fact: This PR allows aliases to function calls for overloaded opIndex.

EDIT: Because it allows overloaded opIndex

@ghost
Copy link
Author

ghost commented Jun 15, 2020

Fun fact: This PR allows aliases to function calls for overloaded opIndex.

Yeah, indeed, it's even part of the tests

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

You're right, overlooked that example.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

In a sense you can consider this PR as

alias A = ArrayExpression(ASymbol, Value).

so the alias is allowed because

  1. alias Symbol = A.Symbol;
  2. enum index = A.Value;

both work. The basic elements of the expression are aliasable so the expression is.

I fully agree with this rule. My concern here is that this PRs adds exceptions, which need to be justified because they break user expectations, e.g.:

int[] a = ....;
int f(int) { ... }
alias b = a[0]; // works
alias c = f(a[0]); // fails?

This does not mean your PR needs to implement everything, it's just necessary to take a look at the bigger picture when adding new features.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

Also, this PR should explain why it's better than current workarounds

EDIT: Which propably exists.

@ghost
Copy link
Author

ghost commented Jun 15, 2020

current workarounds

There's no. Eventually using functions.

should explain

OK. This saves backend time to inline a function that reads or write from an offset.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

There's no. Eventually using functions.

Right, functions / lambdas are kinda akward:

int[2] pipeFd;
pipe(pipeFd);

alias readEnd = pipeFd[0];
alias writeEnd = pipeFd[1];

// <use readEnd, writeEnd>

// Previously
alias readEnd = ref () => pipeFd[0];
alias writeEnd =ref () => pipeFd[1];

// Need parentheses => readEnd(), writeEnd()

OK. This saves backend time to inline a function that reads or write from an offset.

At the expense of more time spent in semantic, so i doubt that there will be much improvement

EDIT: Added ref to those lambdas

@ghost
Copy link
Author

ghost commented Jun 15, 2020

you need to test setter and getter overloads coexisting too... ;),
i.e read and write using your lambdas and same identifier and without parens ;) like a real access to the thing.

EDIT: rewrite all the "runnable" tests with the lambda alternative.

@MoonlightSentinel
Copy link
Contributor

Added the missing refs

@ghost
Copy link
Author

ghost commented Jun 15, 2020

rewrite the tests ;)

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

(I'm not personally arguing against your PR, just exploring it's rationale and consequences s.t. it can convince others as well)

@ghost
Copy link
Author

ghost commented Jun 15, 2020

ok, so

void main(string[] args)
{
    version(none) // current D lang: buzz
    {
///tmp/temp_7F557AF23430.d(11,16): Error: incompatible types for `(__lambda2) == (1337)`: `int delegate() pure nothrow @nogc ref @safe` and `int`
///tmp/temp_7F557AF23430.d(12,9): Error: `rw` is not an lvalue and cannot be modified

        int[1] a = 1337;
        alias rw = ref () => a[0];
        assert(rw == 1337);
        rw = 42;
        assert(a[0] == 42);
    }
    else // with my PR OK
    {
        int[1] a = 1337;
        alias rw = a[0];
        assert(rw == 1337);
        rw = 42;
        assert(a[0] == 42);
    }
}

so test this small program with DMD built with this PR and change version(none) with version(all) to see the difference

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

As said before, you need to use braces parentheses. See https://run.dlang.io/is/VxqbRN

@ghost
Copy link
Author

ghost commented Jun 15, 2020

you need to use braces

Yeah, parens not braces. So this style of alias is not perfect. I tried them before programming what is proposed here. The drawback is that you need to know that the alias is a function, let's say in metaprog code, while with the new shortcut this works transparently as if a variable is read or written (because this is the case).

@MoonlightSentinel
Copy link
Contributor

Good point, easier metaprogramming is always appreciated. (You should probably add a test case for that as well)

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Jun 15, 2020

FYI: This change will need some form of formal approval

CC @WalterBright

@ghost ghost marked this pull request as ready for review June 16, 2020 12:04
@ghost ghost requested a review from WalterBright as a code owner June 16, 2020 12:04
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 16, 2020

Thanks for your pull request, @NilsLankila!

Bugzilla references

Auto-close Bugzilla Severity Description
3546 enhancement Aliasing an element of a static array should be legal if the index is a compile time constant
14128 major AliasDeclaration allows expressions, causing false code for ThisExp

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#11273"

@ghost ghost changed the title fix issue 3546 - experiment aliasing a symbol with a constant indexer fix issue 3546 - Support for alias of individual array elements Jun 16, 2020
@ghost ghost requested a review from ibuclaw as a code owner June 16, 2020 21:48
@ghost
Copy link
Author

ghost commented Jun 18, 2020

That doesn't work as well as it used to (100% nice Monday, 80% nice today). The problem is that AliasDeclaration.toAlias in some context is not necessarily called to build the INdexExp. When this happens, toAlias has to be refused, so that the offset to apply is not lost. For example in test/fail_compilation/index_alias.d, test7, this is why the second alias is refused.

I'll try to see if a new type of symbol can be added, (e.g a new ASTNode...) so that the original var and the offset are tied more strongly, and also to prevent special cases when processing AliasDeclaration.

@ghost ghost marked this pull request as draft June 18, 2020 05:17
@ghost ghost marked this pull request as ready for review June 18, 2020 08:19
@ghost
Copy link
Author

ghost commented Jun 18, 2020

I'll try to see if a new type of symbol can be added, (e.g a new ASTNode...) so that the original var and the offset are tied more strongly

This has worked. No more special case and alias of an alias of "offset symbol" works now.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is useful on occasion. It is consistent with the view that statically-sized arrays and structs have similar behavior. This works today:

struct S {
    int a, b;
}
void main() {
    S s;
    alias y = s.a;
}

A known index in a fixed-length array is the moral equivalent.

Would love to see it generalized further, but perhaps for another day.

@ghost ghost changed the title fix issue 3546 - Support for alias of individual array elements fix issue 3546 - Support for alias of individual array and aggregate elements Jun 26, 2020
@pbackus
Copy link
Contributor

pbackus commented Jul 19, 2020

Allowing aliases to refer to expressions like this is equivalent to adding AST macros.

Consider the following program, which compiles, runs, and prints the indicated output with the changes proposed in this PR:

template counter()
{
    struct Counter
    {
        static size_t count;
        size_t next() { return count++; }
    }

    alias counter = Counter.init.next;
}

void main()
{
    import std.stdio: writeln;

    writeln(counter!()); // 0
    writeln(counter!()); // 1
    writeln(counter!()); // 2
}

This should not be merged without a DIP.

@ghost
Copy link
Author

ghost commented Jul 19, 2020

awesome discovery @pbackus, I really like this example. But yes you're right it's a sort of macro system / expression templates, as in plenty of contexts the source expression is syntax-copied.

Nils Lankila added 2 commits August 16, 2020 07:07
An alias allows to reference a variable. When used this variable is then turned into a `VarExp`.
The idea is why not allowing an extra index, as long as the index is for a variable and that it is a compile-time constant ?
by the way, fix issue 14128
@tgehr
Copy link
Contributor

tgehr commented Aug 28, 2024

Allowing aliases to refer to expressions like this is equivalent to adding AST macros.

Consider the following program, which compiles, runs, and prints the indicated output with the changes proposed in this PR:

template counter()
{
    struct Counter
    {
        static size_t count;
        size_t next() { return count++; }
    }

    alias counter = Counter.init.next;
}

void main()
{
    import std.stdio: writeln;

    writeln(counter!()); // 0
    writeln(counter!()); // 1
    writeln(counter!()); // 2
}

This should not be merged without a DIP.

Well, this works today:

template counter()
{
    struct Counter
    {
        static size_t count;
	static size_t next() { return count++; }
    }

    alias counter = Counter.next;
}

void main()
{
    import std.stdio: writeln;

    writeln(counter!()); // 0
    writeln(counter!()); // 1
    writeln(counter!()); // 2
}

I don't think the example you show demonstrates anything like AST macros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants