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

add __traits(fullyQualifiedName) #3495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WalterBright
Copy link
Member

Complement to dlang/dmd#14711

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@adamdruppe
Copy link
Contributor

What's the use case for this? The definition here is not precise enough for much of anything; what kind of string is it? Is it human readable? Compiler readable? Or a mangle? Does it include template arguments to identify the instance or just the name of the associated symbol?

(And, of course, there probably isn't much of a real world use case even if it is well defined. Code that uses fully qualified names is almost universally an ugly, buggy mess that would be better by all factors if rewritten with proper techniques.)

@WalterBright
Copy link
Member Author

Apparently it's going to flunk DAutoTest until 14711 is merged.

@WalterBright
Copy link
Member Author

What's the use case for this?

Replacing std.traits.fullyQualifiedName which instantiates too many templates, making it the source of a lot of sludge.

@adamdruppe
Copy link
Contributor

std.traits.fullyQualifiedName has no legitimate use case. It should just be deleted without replacement.

@adamdruppe
Copy link
Contributor

Thumbs down emojis btw are among the least valuable things you can do. Instead of making an argument, you just come by and snipe from the sidelines. But here, I'll do your job for you. You might point to the Phobos unittests which, despite being undocumented behavior, point to a possible answer for what the specification should be. (Of course, it is still completely useless, but at least it'd be some kind of definition.)

The phobos unittests try to identify template instances uniquely and reproduce all the details present in the type while expressing them as a kind of pseudo-D source code. The dmd tests copy/paste these, indicating that is probably also the intention here. But... it is pseudo-D, not actual D code, meaning it only actually works for mixin in basic cases (and there's better ways to do mixins than messing with fully qualified names anyway, namely, using local names which are far more reliable, easier to read, and faster to compile), yet people often try to apply it to other things, hence being the code smell. If you wanted something that just worked for mixin, it is possible to define that, but neither Phobos nor dmd does. This would need to be strictly defined output if you wanted to. (But, again, it doesn't enable anything new and encourages anti-patterns.)

Or, is the intention to be a debugging aid, like the compiler would produce for error messages?

The spec doesn't actually say anything, a "string representation" could be absolutely anything. Combined with the anti-patterns this encourages, such an addition to the language would be a clear net-negative. If the intention is to leave the format of the string as implementation-defined behavior just for error message convenience, this should also be called out so everyone knows what to expect.

@adamdruppe
Copy link
Contributor

assert(__traits(fullyQualifiedName, int) == "a 32-bit, signed integer"); should pass in a compliant implementation, since that is a "string representing the type".

@schveiguy
Copy link
Member

Replacing std.traits.fullyQualifiedName which instantiates too many templates, making it the source of a lot of sludge.

Where is FQN used that it causes sludge? That would be a good followup answer to "what is the use case?"

@adamdruppe
Copy link
Contributor

lemme copy/paste some chat stuff before it scrolls away

i think you could define something that does somewhat reliably just works as code to mix in, especially if you used .object.imported!"module.name".other.name consistently (though of course i've never actually seen this be necessary!)

tho there are a few iffy things even in the unittest: static assert(__traits(fullyQualifiedName, typeof(dVarArg)) == "void(...)"); for example is a valid D type... but not valid D code. so is the string supposed to be code?

you only get that if you do typeof(some_function) (note not typeof(&some_function), which is a function pointer type, which is a thing). but if you write it you get an error.

so this is why i ask what the use case is: if it is for making error messages vs making mixin code, you are going to define these things a bit differently

disambiguating error messages is a possible use. and .... then you probably don't want a fqn per se, you want a as-qualified-as-needed-to-identify name. one of the cases that bugs me is something like structFromSql!(import("db.sql")); That fqn is going to be a multi-kilobyte monster since it includes the whole content of db.sql. while technically this is part of the type - change a byte in there and you get a whole new instance - it is rarely helpful to actually read

knowing that it is a structFromSql instantiated on line 55 of file model.d though, that helps a lot

so i think you more often actually want a partially qualified name, either just the name w/o template args, or at least abbreviated template args. eponymous templates too are a bit silly to read, std.algorithm.iteration.map.map doesn't help all that much

and the compiler can potentially do some magic to help with this - it has some logic for showing local name vs qualified name already to disambiguate error messages, which it can determine is necessary by looking for name conflicts in scope, some magic it knows better than the lib. (it doesn't always do a good job but it makes an attempt)

and if that is the intented use case, we can formalize this definition - without necessarily solidifying the output, to allow for future implementations to improve the algorithm without being a breaking change - and reuse some of the error message generation logic

buttttttttt if the intention is something different like using specifically for mixin (which again is something i've never seen code need, every time i see this, i rewrite it with different constructs and get better functionality, cleaner code, and faster builds. it is not a pattern we should encourage), then you probably want overly-verbose, hard to read, but 100% unambiguous even if taken out of context code, e.g. .object.imported!"foo.bar".baz.gz!(.object.imported!"foo.bar".bar, 44UL) and other such horrifying code, but is insulated from the surrounding scope context, cuz it is meant for the compiler to read

@WalterBright
Copy link
Member Author

Where is FQN used that it causes sludge?

What I meant is the process for generating a FQN out of templates produces a lot of templates and memory consumption that isn't useful for much of anything else. In discussions with users, fullyQualifiedName was a big cause of sludge.

@WalterBright
Copy link
Member Author

@adamdruppe __traits(fullyQualifiedName) is intended as a drop-in replacement for std.traits.fullyQualifiedName. This makes it unsurprising that I used the test suite for the latter to test the former. I didn't ask the users why they were using it, but it evidently gets used a lot, and hence is a source of grief from the sludgy side effects of generating FQNs from templates.

The intention is not to invent anything new here, but to help existing users.

I suggest that debating a better way of doing things would be more apropos in the n.g. than here.

@adamdruppe
Copy link
Contributor

I didn't ask the users why they were using it, but it evidently gets used a lot,

Maybe you should cuz this is useless and embedding it in the spec like this just adds more cruft to D.

you say you want to simplify the language but your actions do the opposite.

@adamdruppe
Copy link
Contributor

I'd really like to know who these users are. I'll fix their code for them for free.

I did a couple just a few weeks ago for people who insisted they needed fullyQualifiedName: http://dpldocs.info/this-week-in-d/Blog.Posted_2022_12_26.html#more-mixin-techniques

Notice how the new code is shorter, builds several times faster, and works with more inputs.

I've never - not once - seen a case where people said FQN was necessary where they were actually correct about it. It is a misfeature that encourages bad code.

@schveiguy
Copy link
Member

produces a lot of templates and memory consumption that isn't useful for much of anything else

This is a general problem with templates that needs solving.

@WalterBright
Copy link
Member Author

This is a general problem with templates that needs solving.

Yes, and we've all tried to solve that problem. In the meantime, we do what we can to make it faster.

@adamdruppe
Copy link
Contributor

There have not yet been a single use case described that I haven't been able to debunk in the forum thread.

It is like having a function

void foo() {
    Thread.sleep(5.seconds);
    // do other work
}

And someone says it takes 5 seconds more than it has to to do the work, then someone changes the 5.seconds to 3.seconds and calls it a big win, it is almost twice as fast!

But... the sleep is totally unnecessary to the function's operation. It shouldn't be there at all.

@WalterBright
Copy link
Member Author

@adamdruppe telling people they need to re-engineer their code is not going to work. (It's never ever worked when I've tried it.) People depend on FullyQualifiedName, it makes their compilations slow, so we fix it.

@adamdruppe
Copy link
Contributor

There's two confirmed users of fullyQualifiedName at this point. I already fixed the code for the third one to answer, it took 15 minutes and improved things by every measure. It isn't a grand reengineering, it really is just using "T" instead of fullyQualifiedName!T and maybe refactoring a function to make this easier. I'll even do it for them myself.

I suspect one of those users would be better served by .mangleof and __traits(toType) if they insist on going this silly direction. (which I also objected to as being useless, because it is for some of the same reasons this is useless - you can just use the local name! - but it is there too. How many useless features are you going to add before you listen to what I've been saying for OVER SEVEN YEARS and simply learn use the name that is actually in scope?)

@WalterBright
Copy link
Member Author

@adamdruppe I'm not using FullyQualifiedName myself. Nor did I add it to Phobos. Thank you for fixing user code, if you can get it to zero I'll be happy to deprecate it.

@adamdruppe
Copy link
Contributor

It wouldn't need deprecation if it wasn't enshrined in the compiler in the first place.

But whatever, I was never under any serious illusion you'd actually listen to me. You should still specify the behavior in the PR though if it is going to be added anyway. The current text doesn't even say what kind of string it is, if it is D code or what.

@WalterBright
Copy link
Member Author

It wouldn't need deprecation if it wasn't enshrined in the compiler in the first place.

Library removals also require deprecation.

I was never under any serious illusion you'd actually listen to me

If I didn't care about what you thought, I wouldn't have spent the time explaining the rationale to you. Anyhow, the fact is, I have constraints that you do not have. When something in D goes wrong for users, I am always in the front line receiving the flames. When I remove something, there's always someone who gets very unhappy that I broke their code. It really doesn't matter why, just that their code doesn't work anymore. I'm sympathetic to their point of view, I also get annoyed when my old C code won't compile anymore. There are several things I tried to remove from the language, and was stymied every time.

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.

5 participants