ableC icon indicating copy to clipboard operation
ableC copied to clipboard

Merge returnType into env

Open krame505 opened this issue 8 years ago • 7 comments

Right now we have 2 inherited attributes that get passed around to basicly everything: env and returnType. env contains all the info about the current context of a piece of code, except for the return type, which is annoying. returnType could become part of the env by adding a new namespace on Env.
This wouldn't actually need to be a full-fledged namespace list of Scopes, just a new attribute that would return whatever the innermost return type is, or nothing.

krame505 avatar Feb 27 '17 22:02 krame505

There are contexts where env makes sense but a return type does not. I'm not why trying to push these together is useful?

tedinski avatar Feb 27 '17 22:02 tedinski

The only thing I can find where env occurs but not returnType is Name (unless there are other, similar things), but it could still make sense to think about that a name occurs in a context where return is permitted (or not.) We are tending towards env being the entire state for a tree and not just a list of names with some of our extensions now, so in my opinion it makes more sense to be consistent. Also it is just weird in some places for extension writers to need to provide a return type in some places where it just adds clutter (such as in explicit decoration), or when the env would seem to be the only needed thing; this leads to unexpected flow errors when there seems to be no obvious reason to write a returnType equation when decorating a type expression, but really it is needed.

Anyway, just my take on things, that we should try to keep the number of inherited attributes in the host to a minimum. I'd like to hear what Eric thinks on this as well.

Lucas Kramer

On Mon, Feb 27, 2017 at 4:48 PM, Ted Kaminski [email protected] wrote:

There are contexts where env makes sense but a return type does not. I'm not why trying to push these together is useful?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282882371, or mute the thread https://github.com/notifications/unsubscribe-auth/AIE1ivvJ-iAbE-mLFbf9lVgwYHAlApSGks5rg1KwgaJpZM4MNuNn .

krame505 avatar Feb 27 '17 23:02 krame505

It is true that we're adding more information into env, but I would prefer to keep something separate. return type seems like another form of context that is not something that gets looked up by a name.

Having 2 inh attrs is hardly excessive. I'd leave them separate.

-Eric

On Mon, Feb 27, 2017 at 5:02 PM, Lucas Kramer [email protected] wrote:

The only thing I can find where env occurs but not returnType is Name (unless there are other, similar things), but it could still make sense to think about that a name occurs in a context where return is permitted (or not.) We are tending towards env being the entire state for a tree and not just a list of names with some of our extensions now, so in my opinion it makes more sense to be consistent. Also it is just weird in some places for extension writers to need to provide a return type in some places where it just adds clutter (such as in explicit decoration), or when the env would seem to be the only needed thing; this leads to unexpected flow errors when there seems to be no obvious reason to write a returnType equation when decorating a type expression, but really it is needed.

Anyway, just my take on things, that we should try to keep the number of inherited attributes in the host to a minimum. I'd like to hear what Eric thinks on this as well.

Lucas Kramer

On Mon, Feb 27, 2017 at 4:48 PM, Ted Kaminski [email protected] wrote:

There are contexts where env makes sense but a return type does not. I'm not why trying to push these together is useful?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282882371, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIE1ivvJ-iAbE- mLFbf9lVgwYHAlApSGks5rg1KwgaJpZM4MNuNn>

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282885686, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhd_9vxmpoAWHGrlmTRjT6brkVQ-_uks5rg1X3gaJpZM4MNuNn .

ericvanwyk avatar Mar 02 '17 02:03 ericvanwyk

That is also a reasonable way of looking at things. But I would like to point out then, that if return types don't 'belong' in the env since they aren't actually names, then neither really does extension stuff like the current scope identifier - although in that case there isn't really much choice.

Lucas Kramer

On Wed, Mar 1, 2017 at 8:34 PM, Eric Van Wyk [email protected] wrote:

It is true that we're adding more information into env, but I would prefer to keep something separate. return type seems like another form of context that is not something that gets looked up by a name.

Having 2 inh attrs is hardly excessive. I'd leave them separate.

-Eric

On Mon, Feb 27, 2017 at 5:02 PM, Lucas Kramer [email protected] wrote:

The only thing I can find where env occurs but not returnType is Name (unless there are other, similar things), but it could still make sense to think about that a name occurs in a context where return is permitted (or not.) We are tending towards env being the entire state for a tree and not just a list of names with some of our extensions now, so in my opinion it makes more sense to be consistent. Also it is just weird in some places for extension writers to need to provide a return type in some places where it just adds clutter (such as in explicit decoration), or when the env would seem to be the only needed thing; this leads to unexpected flow errors when there seems to be no obvious reason to write a returnType equation when decorating a type expression, but really it is needed.

Anyway, just my take on things, that we should try to keep the number of inherited attributes in the host to a minimum. I'd like to hear what Eric thinks on this as well.

Lucas Kramer

On Mon, Feb 27, 2017 at 4:48 PM, Ted Kaminski [email protected] wrote:

There are contexts where env makes sense but a return type does not. I'm not why trying to push these together is useful?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282882371, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIE1ivvJ-iAbE- mLFbf9lVgwYHAlApSGks5rg1KwgaJpZM4MNuNn>

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282885686, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOhd_ 9vxmpoAWHGrlmTRjT6brkVQ-_uks5rg1X3gaJpZM4MNuNn> .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-283538333, or mute the thread https://github.com/notifications/unsubscribe-auth/AIE1ilQWpS3xUTOi9q2h0aTevhumQm9_ks5rhirHgaJpZM4MNuNn .

krame505 avatar Mar 02 '17 02:03 krame505

Fair point, but I think that for host language concerns we don't have to put them in the env. But for extensions that need this information for forwarding then we have less choice.

On Wed, Mar 1, 2017 at 8:38 PM, Lucas Kramer [email protected] wrote:

That is also a reasonable way of looking at things. But I would like to point out then, that if return types don't 'belong' in the env since they aren't actually names, then neither really does extension stuff like the current scope identifier - although in that case there isn't really much choice.

Lucas Kramer

On Wed, Mar 1, 2017 at 8:34 PM, Eric Van Wyk [email protected] wrote:

It is true that we're adding more information into env, but I would prefer to keep something separate. return type seems like another form of context that is not something that gets looked up by a name.

Having 2 inh attrs is hardly excessive. I'd leave them separate.

-Eric

On Mon, Feb 27, 2017 at 5:02 PM, Lucas Kramer [email protected] wrote:

The only thing I can find where env occurs but not returnType is Name (unless there are other, similar things), but it could still make sense to think about that a name occurs in a context where return is permitted (or not.) We are tending towards env being the entire state for a tree and not just a list of names with some of our extensions now, so in my opinion it makes more sense to be consistent. Also it is just weird in some places for extension writers to need to provide a return type in some places where it just adds clutter (such as in explicit decoration), or when the env would seem to be the only needed thing; this leads to unexpected flow errors when there seems to be no obvious reason to write a returnType equation when decorating a type expression, but really it is needed.

Anyway, just my take on things, that we should try to keep the number of inherited attributes in the host to a minimum. I'd like to hear what Eric thinks on this as well.

Lucas Kramer

On Mon, Feb 27, 2017 at 4:48 PM, Ted Kaminski < [email protected]> wrote:

There are contexts where env makes sense but a return type does not. I'm not why trying to push these together is useful?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/melt-umn/ableC/issues/25#issuecomment-282882371 , or mute the thread <https://github.com/notifications/unsubscribe-auth/AIE1ivvJ-iAbE- mLFbf9lVgwYHAlApSGks5rg1KwgaJpZM4MNuNn>

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-282885686, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADOhd_ 9vxmpoAWHGrlmTRjT6brkVQ-_uks5rg1X3gaJpZM4MNuNn> .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-283538333, or mute the thread <https://github.com/notifications/unsubscribe-auth/ AIE1ilQWpS3xUTOi9q2h0aTevhumQm9_ks5rhirHgaJpZM4MNuNn> .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/ableC/issues/25#issuecomment-283538916, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhd4XrNmvSmLnO9QGW1qtRpayKSSAFks5rhiurgaJpZM4MNuNn .

ericvanwyk avatar Mar 02 '17 02:03 ericvanwyk

Just wanted to mention another motivation for this that I discovered.
I have found when working on the templating extension that it really is needed in many cases to include a 'resolved' type as a parameter to custom type productions (the refId hack only gets you so far, especially when you are forwarding to something that is injecting globalDecls that are based on something in a header file.) Because of this, we sometimes want to be able to write 'factory' functions to generate types, parameterized by the environment instead of a resolved type. These work by constructing the coresponding type expression, and then decorating it to find the type.
In order to do that decoration, we need to supply returnType as well as the env. We could just set returnType to nothing() since a TypeName that uses returnType somehow to determine its typerep is only possible in some really bizarre ASTs... but to do things right, returnType should be another parameter to these factory productions.
Then another extension writer who wants to use mkVectorType for some reason needs to pass in the current return type, which is extremely non-intuitive.

krame505 avatar Mar 06 '17 19:03 krame505

Hmm, as a side-note, I just realised that we already do have this information in the environment, via currentFunctionItem in the MiscItem namespace. Is this item actually being used for anything? This is never looked up in the host, and I can't seem to find or think of any extension that would utilize this.

krame505 avatar May 19 '17 18:05 krame505