ExprTools.getIdent() also works on CType

[Simn]

This violates the principle of the least surprise: if getString() only works on CString and getInt() only works on CInt, then surely getIdent() can be assumed to only work on CIdent. If you want to support legit upper case identifiers and not introduce another get function, an optional bool argument could be used which determines if CType should be checked too. In my opinion this should default to false, but I'd be fine with a default CType support as long as I can disable it when needed.

[back2dos]

Well, semantically CType has no distinct meaning. It's only there for historical reasons. I am actually trying to hide the difference between CType and CIdent, as the distinction doesn't make sense anymore. Therefore getIdent unifies them transparently, much like drill/resolve do the inverse.

Does this cause actual problems?

[Simn]

I was looking into pattern matching and called a macro like this: MyMacro.with(ExprBlock(x));

The thing here is that ExprBlock (even though it is an enum constructor) gets typed as CType and there was no obvious way of just transforming the x identifier short of checking if the String starts with a lower case letter. Thus my transformer which was supposed to stringy identifiers ended up producing "ExprBlock"("x").

[back2dos]

CType is not about typing. All it says is that the first letter is a capital.

The only thing that the compiler enforces is that type names start in upper case and that's it - which by the way is unrelated to expressions anyway. Casing is user defined for member names, local variables and constructor names.

Try this code:

class Main {
    static function main() {
        var Foo = 5, foo = 5;
        test(Foo);
        test(foo);
    }
    @:macro static function test(e) {
        trace(e);
        return e;
    }
}

If it is your choice to distinguish enum constructors from other values by the case of their first letter, then I think it makes sense that you must check this condition.

Personally I think it's a bad choice for a distinction in the first place. Firstly, enum constructors may be lower case (Bool is the canonical example for that). Secondly, there could be a local variable ExprBlock in the calling scope, that is meant instead of the constructor. If you want to do this properly, you must do this based on type, not on some convention in the hope that client code sticks to it.

Both CType and EType are outdated and thereby have become design flaws, which I would rather not carry into the library. Does that makes sense to you?

[Simn]

I see your point and my example might have been a bad one (I'm just outlining things atm), but this is mainly about expectations that come from method names. If someone uses tink then he most likely has a basic understanding of the expression types and is aware of the difference between CType and CIdent. Such someone is really caught by surprise if a method named getIdent() returns CTypes and it could cause some rather subtle bugs.

I'm undecided if a library like tink should try to overcome compiler design flaws like this. I guess it can be done if library users are made aware of these "fixes", but without inline documentation the lib might violate the "hard to misuse" rule. I'm actually glad you didn't spam the tink sources with an army of comments and function descriptions (I hate that), but maybe a few in-document clarifications on special cases such as this one are in order.

For now I added it to the description of getIdent() on your doc page. Has there been any discussion regarding removal of CType and EType for haxe 3 yet?

[back2dos]

Yes, it's a little aggressive, but personally I did this because I found the distinction of upper and lower case identifiers very confusing and unexpected.

Thanks for updating the doc. I'll see if someone else falls in the trap and put up a warning in the code in that case :D

As for deprecating CType and EType, I don't think it has been discussed yet.

[back2dos]

Oh, there's inline documentation for those methods anyway :D Ok, I've updated it for now. Didn't feel like throwing it out either.