Extern Function Declarations Type Check #10877

[mankyKitty]

Functions declared in an 'extern' were not properly typed checked and
will report syntax errors on failure instead of an ICE.

Based on some guidance from @huonw and @pnkfelix, I've added an additional
argument to force a ident only function declaration check. I believe I've done it so
that unless it's an ForeignItemFn it will use the normal type checking.

A test has been added to compile-fail from the examples in the issue.

Fixes #10877

[huonw]

It would be better to write tests with //~ ERROR ... on every line that should fail. (full docs, section about errors)

[mankyKitty]

Ahh bugger. I had errors on each line on the my first one but saw another compile-fail that used the single error-pattern. Will sort that, and the if test.

[huonw]

Yeah, error-pattern is the old style detector and many of the old tests haven't been updated to avoid it; //~ is far more precise.

[mankyKitty]

Sorry, some more questions on this one before I push the next round of changes up (unless I should do that so others can test?).

I've syntax errors popping up for the expected cases, including the ones @huonw mentioned, is it similar to what you might expect?

extern_patterns.rs:7:9: 7:10 error: expected identifier, found path
extern_patterns.rs:7   fn foo(_: int);
                             ^

I'm also seeing failures on this type of definition within extern blocks:

pub fn printf(_: *u8, ...);

This seems to match with the changes I'm making though, since we're effectively cracking down on the allowed definitions of externs.

All tests pass so far on a make check-stage1. Woo.

[huonw]

Awesome! (Those failure look they are because _ isn't actually an identifier, it's a pattern. I'm not sure whether we should allow _ or not.)

[mankyKitty]

Nevermind, I'm just slow on the uptake. Those one's are meant to fail, the message has been changed because it seems to be failing before it hits the ... pattern. The error message for that function used to be:

//~ ERROR: variadic function must have C calling convention

Now it's as I commented just before. So I would just need to adjust the failure message in the test, I'll try a few more test cases.

[huonw]

Be careful: most failing tests are meant to fail for some specific reason, and unless that reason is because it's actually testing patterns in extern function declarations (i.e. what this PR is changing) then this PR should preserve that behaviour. Specifically, you shouldn't need to touch the //~ ERROR patterns.

For the case above, change it to fn printf(string: *u8, ...); so that it's not the _ pattern that's causing it fail, and the old behaviour is preserved.

[mankyKitty]

That was my first test case, I get the heebies when a new error/warning appears in a file I haven't touched. Long story short, it's still not quite right. Continue the research!

[alexcrichton]

The commits themselves look good to me, but I'm curious what others think about doing this in the parser rather than in typeck somewhere. I would expect this to not modify the grammar of the language itself but rather reject certain valid-syntax programs.

[mankyKitty]

My understanding of the problem was the functions needed to conform to a
particular structure to be valid for type checking. As in the type checking
would be correct already but the structure was wrong, as most of the ways
you can define a function in rust aren't necessarily available to external
callers so aiming for a close match to C style functions was the goal.

No destructuring or pattern matching, or normal awesome tasty rust like
techniques. Keep it simple, keep it safe.

:)

On Wednesday, January 29, 2014, Alex Crichton notifications@github.com
wrote:

The commits themselves look good to me, but I'm curious what others think
about doing this in the parser rather than in typeck somewhere. I would
expect this to not modify the grammar of the language itself but rather
reject certain valid-syntax programs.

Reply to this email directly or view it on GitHubhttps://github.com//pull/11804#issuecomment-33538786
.

[alexcrichton]

This is not just patching up some typechecking code though, this is changing the grammar of the rust language itself. We'd need to go an add special rules for the grammar of an extern function as opposed to a regular function. That's just my opinion though, I may be liking the grammar symmetry with regular functions too much.

[mankyKitty]

Ah I see your point. You could be but symmetry across language components
is something to aim for. :)

Will happily adjust as required, rather do it right the first time!

On Wednesday, January 29, 2014, Alex Crichton notifications@github.com
wrote:

This is not just patching up some typechecking code though, this is
changing the grammar of the rust language itself. We'd need to go an add
special rules for the grammar of an extern function as opposed to a regular
function. That's just my opinion though, I may be liking the grammar
symmetry with regular functions too much.

Reply to this email directly or view it on GitHubhttps://github.com//pull/11804#issuecomment-33540461
.

[mankyKitty]

I'm going to have a try at completing this in the type checker as an alternative, but I was wondering if there was any further thoughts on this from anyone ? @huonw , or @alexcrichton ?

Apologies, I don't mean to pester and I know there is a lot going! :)

[alexcrichton]

I've put this on the meeting agenda for today. (I'm still in favor of the typechecker route over the parser route).

[nikomatsakis]

I'd prefer not only the issue number but also a paragraph explaining in English what the test is driving at. e.g., Test that patterns are disallowed in extern fns.

[nikomatsakis]

(This naturally applies to all the other tests too)

[mankyKitty]

Okey dokey, I'll sort that out.

[alexcrichton]

Closing due to inactivity, but I'd love to see this bug get fixed!