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

[WIP] Enum and Function Parameter Attributes. #105

Closed
wants to merge 27 commits into from

Conversation

skl131313
Copy link

There was some interest in this but not sure what is being done in the shadows, so I'm opening this up for now. I'm not that great at writing these so it's crude and hope to get some feedback on it.

Still need to write the Description, seems this is where some technical stuff like grammar changes go. I'll need to figure that out.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

DIPs/dip10ss.md Outdated

### Links

[Does it require a DIP?](http://forum.dlang.org/thread/[email protected])
Copy link
Member

Choose a reason for hiding this comment

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

Event though this is the title of the thread, better use a more descriptive title here, e.g. "Preliminary discussion about Enum and Function Parameter Attributes on the NG"

DIPs/dip10ss.md Outdated

## Rationale

It is currently not possible to attach attributes to both enums and function parameters. This excludes a few features that can be used with almost any other symbol in D. Attributes and User Defined Attributes serve as a means to provide extra meta data for a symbol. What can be said for why attributes were included as a feature in D can also be said for why they should be extended to enums and function parameters. It is benefitial to provide extra meta data about a symbol that can be used at compile-time.
Copy link
Member

Choose a reason for hiding this comment

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

"user-defined" is the official spelling, see dlang/dlang.org#2003 (comment)

DIPs/dip10ss.md Outdated
## Rationale

It is currently not possible to attach attributes to both enums and function parameters. This excludes a few features that can be used with almost any other symbol in D. Attributes and User Defined Attributes serve as a means to provide extra meta data for a symbol. What can be said for why attributes were included as a feature in D can also be said for why they should be extended to enums and function parameters. It is benefitial to provide extra meta data about a symbol that can be used at compile-time.

Copy link
Member

Choose a reason for hiding this comment

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

I would mention "turtles all the way down" or the professional term orthogonality of language features.

DIPs/dip10ss.md Outdated
deprecated("use feature0 or feature1 instead")
feature2,
}
```
Copy link
Member

Choose a reason for hiding this comment

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

This is a good example where this was desperately needed: vibe-d/vibe.d#1947

DIPs/dip10ss.md Outdated
Licensed under [Creative Commons Zero 1.0](https://creativecommons.org/publicdomain/zero/1.0/legalcode.txt)



Copy link
Member

Choose a reason for hiding this comment

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

Remove these empty lines?

DIPs/dip10ss.md Outdated
{
// ...
}
```
Copy link
Member

Choose a reason for hiding this comment

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

I had two good examples for Vibe.d:

@body("user")
@errorDisplay
auto postUsers(User _user, string  _error)

beomes

auto postUser(@body User user, @errors Errors errors)

@path("/users/:id")
@queryParam("page")
@before!authenticate("user")
auto getUsers(string _id, int page, User user)

becomes

@path("/users/:id")
auto getUser(@urlParam string id, @queryParam int page, @auth User user)

@wilzbach
Copy link
Member

wilzbach commented Jan 1, 2018

Also note that this topic has often come up & there's even an implementation: dlang/dmd#4783

@skl131313
Copy link
Author

That implementation is quite old though. Looks like it was when the frontend was still written in C++.

@JinShil
Copy link
Contributor

JinShil commented Jan 2, 2018

@skl131313 There's some good information in dlang/dmd#4783 that you may be able to use in this DIP.

  • It's good to just link it as a reference so readers can get a more holistic perspective, and show that you have done your homework when writing this DIP.
  • Read through the comments and see if there are any questions, concerns, and/or ideas that you can use to further motivate or clarify this DIP
  • DMD was automatically translated from C/C++ to D, so porting the code in that PR to DMD's current code base will still be rather mechanical.

@wilzbach
Copy link
Member

wilzbach commented Jan 2, 2018

DMD was automatically translated from C/C++ to D, so porting the code in that PR to DMD's current code base will still be rather mechanical.

Yes it is. I actually just started doing that, so be prepared for an incoming PR with the "translated" PR ;-)

@wilzbach
Copy link
Member

wilzbach commented Jan 2, 2018

Yes it is. I actually just started doing that, so be prepared for an incoming PR with the "translated" PR ;-)

dlang/dmd#7576 ... so now a part of this DIP (having UDAs for function parameters feature) is just a click on "merge" button away.
(Of course, we still need the DIP and respective grammar changes).

@skl131313
Copy link
Author

@wilzbach
Sweet, thanks for putting in the work for getting it up to date.

@mdparker
Copy link
Member

mdparker commented Mar 7, 2018

@skl131313 Sorry to keep you waiting with this. Now that the new procedures are approved, I'm getting things moving again. I want to get the older submissions cleared out first. Once those are gone, I'll start looking at this one when you are ready. I ask that you please modify it to reflect the new Template.md.

Thanks for your patience.

@mdparker
Copy link
Member

@skl131313 Looks like you're up. If you're ready to go, let me know and I'll request Draft Review feedback in the forums. Let's get the content nailed down first then I'll start my editing passes.

@skl131313
Copy link
Author

Yah I guess, I am a bit busy right now though. Was thinking of adding something about lambda functions though. That's something that isn't mentioned anywhere.

@wilzbach
Copy link
Member

BTW I just found this existing, stalled PR for attributes at enum members: dlang/dmd#6161

So we have all the pieces in place and just need the formal approval to merge them :)

DIPs/dip10ss.md Outdated
}
```

A solution for applying the `deprecation` attribute to an enum member can be done by reimplementing an enum as a structure with static enums. This allows the attribute to be placed with the desired enum member. While still allowing for any existing code that simply used an enum before hand to still work accordingly, as if the struct was an enum.
Copy link
Contributor

Choose a reason for hiding this comment

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

"beforehand" is one word.

DIPs/dip10ss.md Outdated
| DIP: | (number/id -- assigned by DIP Manager) |
| Review Count: | 0 (edited by DIP Manager) |
| Author: | [@skl131313](https://github.com/skl131313) |
| Implementation: | [(Partial) Function UDAS](https://github.com/dlang/dmd/pull/7576) |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll want to add link to the enum PR here: dlang/dmd#6161

@mdparker
Copy link
Member

Feedback from the announcement: https://forum.dlang.org/post/[email protected]

Please keep any responses here instead there.

DIPs/dip10ss.md Outdated
// through existing functionality of __parameters
static if(is(typeof(someFunction) PT == __parameters))
{
static assert(__traits(getAttributes, PT[0 .. 1]) == 93);
Copy link

Choose a reason for hiding this comment

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

wouldn't it be PT[0] == 93 or PT[0..1]=[93]?

Copy link
Author

@skl131313 skl131313 Mar 21, 2018

Choose a reason for hiding this comment

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

PT is the parameter type, I don't know why but I don't think it works with just [0] iirc. It was missing the [0] at the end of the __traits() though.

@skl131313
Copy link
Author

skl131313 commented Mar 21, 2018

@MetaLang

IMO, the abstract is not right. This may differ between institutions, but the way I was taught to write abstracts is that they should be a condensed summary of what your document is about. Think of it as an overview so someone can quickly read it to get an idea of what you'll be talking about through the rest of the document.

I was never good at writing abstracts, I sort of went by what the other DIPs did. It sort of does give a very brief outline of what the DIP hopes to achieve, I reworded it a bit, but doesn't seem right to me still.

Also, this is just a copy&paste of the same example from the "Existing Solutions" section; I don't think the whole bit with creating a struct with enum members, etc. really needs to be repeated.

Yah I don't like the repetition but in the Examples it shows what the code would become, and in existing solutions it explains how it is currently implemented. Without the code in existing solutions it makes it difficult to explain. If you have a suggestions ? I can just move the example further down the list to make it less obvious.

All of these examples except the first seem to rely on function parameter attributes being accessible outside that function. If I recall correctly, this caused some controversy in past discussion which must be addressed in this DIP.

Do you have an example or link as to why that is? The most useful use cases are accessing them from external functions. If parameter UDAs shouldn't be accessible from external sources you could similarly say the same for struct variables and their corresponding UDAs. I'll add a bit about that, maybe once I have a better understanding of the argument though.

@mdparker
Copy link
Member

@skl131313 We'll get the wordsmithing sorted before we move forward to the community review. FYI, I'll give a few more days for draft review feedback, then see where we are on Monday.

@mdparker
Copy link
Member

mdparker commented Mar 26, 2018

@skl131313 As a next step, let's work on fleshing out that abstract, shall we?

Take at look at the abstracts for DIPs 1005 and 1009. Both of them define the features they are touching as they currently exist in the language, then summarize the changes they intend to make.

Your DIP touches on attributes, so you could start out with a short description of what they are and how they are used, being sure to highlight that they are currently applicable only to functions. Then you can finish it off by explaining how this DIP proposes to expand the usage of attribute by allowing their application to enum and function parameters.

With that done, you can then expand the opening to the Rationale, but let's get the abstract finished first.

@skl131313
Copy link
Author

Well I did change it a bit, but I feel there is excessive duplication of the same information.

@mdparker
Copy link
Member

That's much better. I've made an editing pass to tighten it up a bit. Take as much of it as you'd like:

In D, attributes provide a means of attaching compile-time information to a symbol. Built-in attributes provide information to the compiler, User-Defined Attributes (UDAs) provide information to the programmer or tooling, and both are accessible through compile-time reflection.

D currently prohibits the use of attributes on some types of symbols, specifically enums and function parameters. This document describes a proposal to allow UDAs and built-in attributes to be attached to enums and function parameters.

@mdparker
Copy link
Member

I feel there is excessive duplication of the same information.
I suppose you mean in relation to the Rationale. That's our next target.

The abstract is the "what", and after your edit it now fills that role. The rationale is the "why". It doesn't need to repeat the abstract, so you can remove all of that redundant bits from the rationale. Instead, you need to focus on summarizing the problem the proposal is trying to solve, i.e. attributes cannot currently be applied to enums and function parameters. Why is this a bad thing? What are the negative consequences? What benefit(s) could come from allowing this feature?

Currently, you have this:

Attributes and user-defined attributes (UDA) serve as a means to provide extra metadata for a symbol. The reasoning for why attributes were included as a feature in D can be included as to why UDAs should be extended to enums and function parameters. It is benefitial to provide extra metadata about a symbol that can be used at compile-time.

There is nothing concrete here supporting why there's a problem or any benefits that will come from implementing this proposal. It's too vague. What is "the reasoning why attributes were included as a feature"? Why is it beneficial "to provide extra metadata about a symbol"? We need to be clear and precise. When you've got some concrete points laid out, you can use before & after code snippets for clarity.

I'm sure you had a use case that prompted this DIP. You might use that as a starting point.

@mdparker
Copy link
Member

mdparker commented Apr 9, 2018

@skl131313 Can you do me a favor and contact me at [email protected]? I need to discuss something with you. Thanks!

@mdparker
Copy link
Member

This DIP is no longer required. The decision has been made to implement the feature without the DIP.

@mdparker mdparker closed this Apr 12, 2018
@wilzbach
Copy link
Member

wilzbach commented Jul 2, 2018

BTW @mdparker this DIP is a great resources and as both PRs have been merged in master now it would be a very helpful if we could still give it a DIP number (or at least merge it into master, s.t. it shows up in Google) as looking into the future this draft DIP (and its motiviation) is hard to find.

@mdparker
Copy link
Member

mdparker commented Jul 2, 2018

I wouldn't want to assign it a number. I suppose I could create a folder for DIPs that are determined to be unnecessary.

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

Successfully merging this pull request may close these issues.

5 participants