Disallow type parameter names shadowing other types. #1526

tokenrove · 2017-01-22T02:31:38Z

(I'm opening this PR as is, but admittedly this is probably the wrong way to solve this, thus my explicit use of WIP in the commit message. If I can get some guidance on the right way to fix this, I'd be more than happy to contribute a suitable fix, including a more detailed commit message.)

So, this started with me trying to do something stupid (and getting a segfault, on the release compiler), but boiled down, this is sufficient to cause an assert with a debug compiler:

class A[I8]
  let b: U8 = 0

What seems to happen is that if the type parameter for A is any of the types named in _str_uif_types in libponyc/expr/literals.c, when we then go to look up U8 (which can be any other type from that same set of builtin types) in uifset_simple_type(), the structure of the AST we get back is not what we expect (it's become a TK_TYPEPARAMREF thanks to the call to names_nominal() within types_builtin() and friends). As a result ast_childidx(uif, 3) returns NULL and ast_setid() aborts (or goes on to segfault, in release mode).

I don't know enough about either Pony or the compiler to know what the correct behavior should be, but I imagine that having I8 as a type parameter here should either be forbidden, or it should shadow I8 throughout this definition, and not affect uifset_simple_type() at all. It seems like other parts of the compiler do the right thing, and the presence of literal type inference causes the snafu, since changing 0 to U8.min_value() works fine.

Advice would be appreciated, even "go read this part of the code".

jemc · 2017-01-23T19:10:37Z

@tokenrove Thanks for bringing the issue to our attention!

We probably need to discuss and decide on a desired behaviour here.

Because Pony disallows shadowing in many other cases where it may cause confusion/bugs, I'd argue that we should disallow a type parameter name from shadowing any type name in the current file scope (whether the type is in builtin, or imported into the file scope via a use). This should help us to avoid the specific problem you found, as well as other problems that could pop up in the future due to this kind of shadowing.

@sylvanc @SeanTAllen @Praetonus Thoughts?

Praetonus · 2017-01-23T20:21:36Z

I agree on disallowing name shadowing from type parameters.

jemc · 2017-01-23T21:33:10Z

The other upside is that if we disallow all shadowing by type parameter names we can also use the compiler error as an opportunity to provide a useful hint to the user who made the mistake and is probably confused about type parameters vs type arguments.

When a type parameter is introduced which matches any of the simple UIF names, it causes any other such simple type to be recognized as a type parameter reference, which results in the compiler aborting. In the discussion on ponylang#1526, it was decided that all shadowing by type parameters should be prohibited. Hence, in this patch, if we're resolving a type param whose id is still nominal, we check the nearest module to see if that name is already defined.

tokenrove · 2017-01-24T13:44:33Z

I've updated this to check the enclosing module scope for the name of a type param during the names phase.

Let me know if this is on the right track or if I should take a different approach. Suggestions for more test cases would be helpful, too.

tokenrove · 2017-01-24T13:46:57Z

Oh, also, let me know if I should keep the TYPEPARAMREF check in the UIF code; it seems like it should now be unnecessary, but maybe some larger sanity check that type_builtin returns what we expect in that code might allow a better error message for other, similar cases if any exist.

jemc · 2017-01-24T19:28:42Z

I've removed the "needs discussion during sync" label, as I think the agreement between @Praetonus and I should be sufficient consensus to move forward.

jemc

Looks great, and I agree the names pass is the right place to get this check done.

I just have a few small style comments.

jemc · 2017-01-24T19:30:14Z

src/libponyc/pass/names.c

@@ -178,6 +178,15 @@ static bool names_typeparam(pass_opt_t* opt, ast_t** astp, ast_t* def)
    return false;
  }

+  if (ast_id(ast) == TK_NOMINAL) {
+    ast_t* module = ast_nearest(ast, TK_MODULE);
+    if (module && ast_get(module, ast_name(id), 0)) {


Can you fix the style/formatting of both of these if statements to match the surrounding code style?

Specifically, no space between the if and the (, and the { should be on the next line.

It's not my favorite style for if statements, but it's the style we've got 😉.

jemc · 2017-01-24T19:31:08Z

src/libponyc/pass/names.c

+    ast_t* module = ast_nearest(ast, TK_MODULE);
+    if (module && ast_get(module, ast_name(id), 0)) {
+      ast_error(opt->check.errors, def,
+                "Type parameter shadows existing type");


The code style for this error message should be just indented two spaces in from the previous line, as we do in the ast_error in the code block above.

How embarrassing. I even noted all the oddities (from my perspective) of the style and then promptly forgot to observe them when I revised my changes. Thanks for the heads up.

jemc · 2017-01-24T19:32:31Z

test/libponyc/badpony.cc

+{
+  const char* src =
+      "class A[I8]"
+        "let b: U8 = 0";


These two lines seem to have extra indentation. Please match the style of the test above, using two spaces for each new level of indentation.

jemc · 2017-01-24T19:35:20Z

Suggestions for more test cases would be helpful, too.

It might be useful to add another test where the shadowed type name was declared in the same file.

For example:

class B
class A[B]

When a type parameter is introduced which matches any of the simple UIF names, it causes any other such simple type to be recognized as a type parameter reference, which results in the compiler aborting. In the discussion on ponylang#1526, it was decided that all shadowing by type parameters should be prohibited. Hence, in this patch, if we're resolving a type param whose id is still nominal, we check the nearest module to see if that name is already defined.

tokenrove · 2017-01-24T23:52:51Z

I noticed that there doesn't seem to be a consistent style in error messages, but there seemed to be more which started with a lowercase letter than uppercase, so I revised the message.

The suggested test case is a great idea, but already gets caught in the scope pass. I added it anyway, with the error from scope, but I'm happy also to ditch it if this seems extraneous.

Offhand, it's not a big deal, but it would be nice if there was a style lint and a suggested emacs setup for editing the C source. Maybe these exist and I missed them.

jemc · 2017-01-25T19:18:20Z

I noticed that there doesn't seem to be a consistent style in error messages, but there seemed to be more which started with a lowercase letter than uppercase, so I revised the message.

Yes, I've noticed the varying style as well, and agree that we should clean them up to be consistent. We'd definitely be open to a PR that cleans up the error messages for consistency.

Offhand, it's not a big deal, but it would be nice if there was a style lint and a suggested emacs setup for editing the C source. Maybe these exist and I missed them.

I don't deal a lot with automated style linting, as most projects I work heavily on have a style that's more subjective and difficult to automate for. For styles like that, I prefer to just work socially and pay attention to surrounding code to match style.

However, the C source for this project does have a very mechanical style, so I would definitely be open to someone adding a automated style linting tool configuration (for the C source only, I think). As long as it doesn't add any required dependencies for building the project, I think it should be okay.

Since I'm going to merge this PR, I'll open a ticket for the style linting question, mention you in it, and mark it for discussion on today's sync call to see if anyone is opposed to such an initiative.

jemc · 2017-01-25T19:23:45Z

Thanks for your work on this!

tokenrove · 2017-01-26T04:25:09Z

Thanks for your patience!

jemc added bug: 2 - under investigation labels Jan 23, 2017

tokenrove force-pushed the wip-simple-type-param-shadow branch from 601f6a2 to 235cc4f Compare January 24, 2017 13:43

jemc added bug: 4 - in progress and removed bug: 2 - under investigation labels Jan 24, 2017

jemc requested changes Jan 24, 2017

View reviewed changes

tokenrove force-pushed the wip-simple-type-param-shadow branch from 235cc4f to 4bec9ad Compare January 24, 2017 23:43

jemc approved these changes Jan 25, 2017

View reviewed changes

jemc mentioned this pull request Jan 25, 2017

Automated style linting for C source code #1532

Open

jemc changed the title ~~WIP: Prevent abort when type param is simple~~ Disallow type parameter names shadowing other types. Jan 25, 2017

jemc added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed bug: 4 - in progress labels Jan 25, 2017

jemc merged commit fa95291 into ponylang:master Jan 25, 2017

ponylang-main added a commit that referenced this pull request Jan 25, 2017

Update CHANGELOG for PR #1526

fe0433a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow type parameter names shadowing other types. #1526

Disallow type parameter names shadowing other types. #1526

tokenrove commented Jan 22, 2017

jemc commented Jan 23, 2017 •

edited

Loading

Praetonus commented Jan 23, 2017

jemc commented Jan 23, 2017

tokenrove commented Jan 24, 2017

tokenrove commented Jan 24, 2017

jemc commented Jan 24, 2017

jemc left a comment

jemc Jan 24, 2017

jemc Jan 24, 2017

tokenrove Jan 24, 2017

jemc Jan 24, 2017

jemc commented Jan 24, 2017

tokenrove commented Jan 24, 2017

jemc commented Jan 25, 2017

jemc commented Jan 25, 2017

tokenrove commented Jan 26, 2017

Disallow type parameter names shadowing other types. #1526

Disallow type parameter names shadowing other types. #1526

Conversation

tokenrove commented Jan 22, 2017

jemc commented Jan 23, 2017 • edited Loading

Praetonus commented Jan 23, 2017

jemc commented Jan 23, 2017

tokenrove commented Jan 24, 2017

tokenrove commented Jan 24, 2017

jemc commented Jan 24, 2017

jemc left a comment

Choose a reason for hiding this comment

jemc Jan 24, 2017

Choose a reason for hiding this comment

jemc Jan 24, 2017

Choose a reason for hiding this comment

tokenrove Jan 24, 2017

Choose a reason for hiding this comment

jemc Jan 24, 2017

Choose a reason for hiding this comment

jemc commented Jan 24, 2017

tokenrove commented Jan 24, 2017

jemc commented Jan 25, 2017

jemc commented Jan 25, 2017

tokenrove commented Jan 26, 2017

jemc commented Jan 23, 2017 •

edited

Loading