Use rosetree to store capability type. #212

albertnetymk · 2015-08-24T00:52:03Z

No description provided.

TobiasWrigstad · 2015-09-15T20:26:17Z

What is this?

albertnetymk · 2015-09-16T17:38:08Z

Traits implemented by one class are combined using + and * operators. Before, only + is supported, and they are stored in a list. Now in order to support two operators, the structure needs to be saved. This PR use rose tree to save all the traits implemented.

TobiasWrigstad · 2015-09-16T19:00:55Z

OK. So, is it fair to say that this PR should be renamed "Adding conjunction composition for traits"?

TobiasWrigstad · 2015-09-22T14:44:13Z

Bump @EliasC …

albertnetymk · 2015-09-22T15:43:03Z

OK. So, is it fair to say that this PR should be renamed "Adding conjunction composition for traits"?

I would be reluctant to call it so, for "adding conjunction composition" implies that typechecker catches malformed usages and emit correct C code. On the contrary, this PR only adds the rose tree data structure to store all the traits.

EliasC · 2015-09-23T14:17:15Z

src/types/Types.hs

+    map TraitType ((toList . typeTree) capability)
+typesFromCapability ty =
+    error $ "Types.hs: Can't get the traits of non-capability type "
+      ++ showWithKind ty


I like the name traitsFromCapability better since it specifies the granularity of the resulting types. It is not clear to me (without reading the code) what typesFromCapability gives me for A * (B + C), while with traitsFromCapability it's obvious that it is [A, B, C] (in some order).

albertnetymk · 2015-09-23T14:35:58Z

Have looked at all the cases where typesFromCapability is used, the prefix types feels OK, for the return type is [Type] anyway.

EliasC · 2015-09-23T14:40:30Z

src/types/Types.hs

+      let
+        par_types = map toList $ map (fmap TraitType) ts
+      in
+        concatMap collect ts ++ [par_types]


conjunctiveTraitsFromCapability is a better name I think, if this function takes a conjunctive type A*B and returns [[A], [B]] (I don't see how the third level of list-nesting is used, but maybe you get the point). parallelTypes is confusing since we have a Par type.

EliasC · 2015-09-23T14:42:18Z

Have looked at all the cases where typesFromCapability is used, the prefix types feels OK, for the return type is [Type] anyway.

The return type just strengthens my point I think. Reading the name and the type I have no way of knowing that all the types in the resulting list are actually trait types!

albertnetymk · 2015-09-23T14:42:41Z

Yes, conjunctive is surely better.

EliasC · 2015-09-23T14:43:55Z

Yes, conjunctive is surely better.

Could you give a quick explanation of what it returns as well? I didn't get why it's a list of lists of lists.

EliasC · 2015-09-23T14:48:11Z

src/types/Types.hs

+
+instance Traversable RoseTree where
+  traverse f (Leaf i) = Leaf <$> f i
+  traverse f (RoseTree op ts) = RoseTree op <$> traverse (traverse f) ts


It's nice how the structure is made instances of these type classes so that normal map, mapM, etc. works!

albertnetymk · 2015-09-23T15:01:16Z

Reading the name and the type I have no way of knowing that all the types in the resulting list are actually trait types!

That's true, but the function name could be changed to arbitrary strings, while the signature is checked all the time. It's good to rely on the signature instead of the name. BTW, we don't have trait type, only TraitType value constructor.

I didn't get why it's a list of lists of lists.

I think it's because there are multiple levels of conjunctive traits, since it's a tree.

EliasC · 2015-09-23T15:05:21Z

That's true, but the function name could be changed to arbitrary strings, while the signature is checked all the time.

That's hardly an argument to choose bad names though :)

I will drop this for now and bring it up again if it causes problems while programming.

EliasC · 2015-09-23T15:06:47Z

src/types/Typechecker/Typechecker.hs

+
+    is_val :: FieldDecl -> TypecheckM ()
+    is_val f = unless (isValField f) $ tcError $
+      printf "'%s' is not val field" $ show f


This error message should be improved (something like Disjunctive traits A and B cannot have overlapping mutable field requirements), but I'm ready to push that to a later commit.

EliasC · 2015-09-23T15:15:56Z

I think it's because there are multiple levels of conjunctive traits, since it's a tree.

So something like this?

The innermost lists are collection of disjunctive traits that may share fields. The sibling lists of those lists contain traits that are all conjunctive between lists. Each list of these lists contain the traits of different levels of the tree.

It feels like you should be able to do this with only two levels so that only the first two "steps" above are needed. Please let me know if you can shed any light on this!

EliasC · 2015-09-23T15:16:30Z

When the discussed issues are fixed, I think this is ready to be merged!

EliasC · 2015-09-23T15:16:39Z

(+rebasing)

albertnetymk · 2015-09-23T15:29:31Z

It feels like you should be able to do this with only two levels so that only the first two "steps" above are needed.

It's possible that I overcomplicated the situation; for A*B + C*D, it returns something like this I believe:

[[[A],[B]],[[C],[D]]]

EliasC · 2015-09-24T08:21:20Z

Thanks for the explanation! Let's leave it as it is and refactor it if we find a need for it.

There is still a spelling mistake in Util.hs. resovle_capa should be resolve_capa (or rather resolveCapability, since that is the naming convention for the rest of the surrounding function!). After this I think the PR is ready to be merged.

One comment on the error message of overlapping fields though. Right now the message is

'f : T' needs to be defined as val field in both conjunctive traits 'Foo' and 'Bar'

which describes a solution to get rid of the error, but not the reason there was an error in the first place. I suggest instead

Conjunctive traits 'Foo' and 'Bar' cannot share mutable field 'f : T'

which describes what design choice lead to the error, which is more useful in teaching the programmer how to avoid errors like this in the future. If you want to also describe a solution you could add Consider turning it into a val-field afterwards.

albertnetymk · 2015-09-24T08:32:17Z

I have found the idea of using camel and snake to distinguish the public and private functions very helpful. Encourage others to try it out.

kikofernandez · 2015-09-24T08:59:49Z

I would rather suggest that we come up with some general guidelines for writing code, instead of everyone choosing what to do. Now, @albertnetymk suggests to use snake case for private functions. I suggest that everyone uses snakel case: this_Is_Snakel_Case... hope everyone gets it :)

EliasC · 2015-09-24T09:03:36Z

@albertnetymk For the nth time point you to our own guidelines for contibuting:

Respect conventions

You might love tabs in your source code. You might
really, really love them. But if re submitting a pull request to a project that
uses spaces, you're going have to suck it up and use them.

Aside from coding style, projects might have more subtle conventions that it
makes sense to follow. Perhaps private functions never have docstrings, or
perhaps commit messages are always written in the imperative, present tense. Try
to ensure your contributions to the project don't stand out as
unusual.

Although I prefer camelCase (as you know), that's not why I'm arguing that you should use it. My point is that you need to respect the conventions of the code you're contributing to. Here is the where clause in question:

          resovle_capa :: Type -> TypecheckM Type
          resovle_capa t
            | emptyCapability t = return t
            | singleCapability t = resolveType $ head $ typesFromCapability t
            | otherwise =
              mapM_ resolveSingleTrait (typesFromCapability ty) >> return ty

          resolveSingleTrait t = do
            result <- asks $ traitLookup t
            when (isNothing result) $
                   tcError $ "Couldn't find trait '" ++ getId t ++ "'"

All the functions visible here are have descriptive names in camelCase without abbreviations. When you add something to this context and ignore these conventions, you deteriorate the quality of the code. It would make more sense to rename resolveSingleTrait into resovle_sngl_trt so that there at least is a convention locally (but it should be obvious that this would not improve the code).

I agree with @kikofernandez. We need to agree on a convention that we can all follow. It is obvious that these discussions have no effect.

EliasC · 2015-09-24T09:21:30Z

I will merge this now, but we should continue the discussion about coding conventions. I started an issue for it: #224

Use rosetree to store capability type.

kaeluka · 2015-09-24T10:05:53Z

I agree with @kikofernandez. We need to agree on a convention that we can all follow. It is obvious that these discussions have no effect.

In this specific case, we have the convention -- you linked to it. @albertnetymk just doesn't adhere to it, so this is a problem of programmer discipline/weak enforcing, not lack of convention.

kaeluka · 2015-09-24T21:34:31Z

src/types/Types.hs

+  fmap f (Leaf i) = Leaf $ f i
+  fmap f (RoseTree op ts) = RoseTree op $ map (fmap f) ts
+
+instance Foldable RoseTree where


This is breaking the build, Foldable is in Prelude only since ghc 7.10

https://wiki.haskell.org/Foldable_Traversable_In_Prelude

kikofernandez added the ready for review label Sep 22, 2015

albertnetymk added in progress and removed ready for review labels Sep 22, 2015

EliasC reviewed Sep 23, 2015
View reviewed changes

albertnetymk added 2 commits September 23, 2015 20:09

Use rosetree to store capability type.

43b2aeb

Report conjunctive traits on common non-val fields

19ee3f3

albertnetymk added ready for review and removed in progress labels Sep 24, 2015

Update error msg for non-val fields in conjunctive traits

5b20424

EliasC added a commit that referenced this pull request Sep 24, 2015

Merge pull request #212 from albertnetymk/rosetree

aa0aeb9

Use rosetree to store capability type.

EliasC merged commit aa0aeb9 into parapluu:master Sep 24, 2015

EliasC removed the ready for review label Sep 24, 2015

albertnetymk deleted the rosetree branch September 24, 2015 10:41

kaeluka reviewed Sep 24, 2015
View reviewed changes

EliasC mentioned this pull request Oct 6, 2015

Coding Conventions #241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rosetree to store capability type. #212

Use rosetree to store capability type. #212

albertnetymk commented Aug 24, 2015

TobiasWrigstad commented Sep 15, 2015

albertnetymk commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

TobiasWrigstad commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

EliasC Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC Sep 23, 2015

EliasC commented Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC commented Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC commented Sep 24, 2015

albertnetymk commented Sep 24, 2015

kikofernandez commented Sep 24, 2015

EliasC commented Sep 24, 2015

Respect conventions

EliasC commented Sep 24, 2015

kaeluka commented Sep 24, 2015

kaeluka Sep 24, 2015

Use rosetree to store capability type. #212

Use rosetree to store capability type. #212

Conversation

albertnetymk commented Aug 24, 2015

TobiasWrigstad commented Sep 15, 2015

albertnetymk commented Sep 16, 2015

TobiasWrigstad commented Sep 16, 2015

TobiasWrigstad commented Sep 22, 2015

albertnetymk commented Sep 22, 2015

EliasC Sep 23, 2015

Choose a reason for hiding this comment

albertnetymk commented Sep 23, 2015

EliasC Sep 23, 2015

Choose a reason for hiding this comment

EliasC commented Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC Sep 23, 2015

Choose a reason for hiding this comment

albertnetymk commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC Sep 23, 2015

Choose a reason for hiding this comment

EliasC commented Sep 23, 2015

EliasC commented Sep 23, 2015

EliasC commented Sep 23, 2015

albertnetymk commented Sep 23, 2015

EliasC commented Sep 24, 2015

albertnetymk commented Sep 24, 2015

kikofernandez commented Sep 24, 2015

EliasC commented Sep 24, 2015

Respect conventions

EliasC commented Sep 24, 2015

kaeluka commented Sep 24, 2015

kaeluka Sep 24, 2015

Choose a reason for hiding this comment