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

Functions with class-specific modifiers are allowed in global scope #797

Closed
alexnask opened this issue Aug 18, 2014 · 15 comments · Fixed by #800
Closed

Functions with class-specific modifiers are allowed in global scope #797

alexnask opened this issue Aug 18, 2014 · 15 comments · Fixed by #800
Assignees
Milestone

Comments

@alexnask
Copy link
Collaborator

For example

f: abstract func { "test" println() }
f()

Compiles and runs fine.
Should this be allowed behavior?
I'm tagging this as bug because I think it is not expected behavior, can you confirm @fasterthanlime ?

Edit:
Here is one more example with the three modifiers that should be disallowed

f: abstract func

g: static func

h: final func
@alexnask alexnask self-assigned this Aug 18, 2014
@fasterthanlime
Copy link
Collaborator

Definitely not expected behavior, nonsensical modifiers should err outside
of class declarations.

On Mon, Aug 18, 2014 at 2:50 PM, Alexandros Naskos <notifications@github.com

wrote:

For example

f: abstract func { "test" println() }f()

Compiles and runs fine.

Should this be allowed behavior?
I'm tagging this as bug because I think it is not expected behavior, can
you confirm @fasterthanlime https://github.com/fasterthanlime ?


Reply to this email directly or view it on GitHub
#797.

@alexnask
Copy link
Collaborator Author

Ok, I'm on to it next after implementing abstract operators :)

@alexnask alexnask changed the title Abstract functions are allowed in the global scope Functions with class-specific modifiers are allowed in global scope Aug 18, 2014
@alexnask
Copy link
Collaborator Author

I've fixed this but on a branch that sits on top of the abstract operator changes, so I'm waiting for code review on that pull request before I open this one :)

@fasterthanlime
Copy link
Collaborator

Ahhhhh got no time to review - can you merge & I'll review later? master is unstable now so it's okay(TM).

(Or maybe @zhaihj wants to review?)

@alexnask
Copy link
Collaborator Author

Well the tests and Travis-CI build pass and I trust the code has no side-effects, I guess I'm just treading carefully until I get accustomed to rock once more :P

Anyway, I'm merging, those are pretty trivial changes

@davidhesselbom
Copy link
Contributor

I had a file like this:

import ./internal
import math

extend Float {
    absolute: static func(value: This) -> This {
        value >= 0 ? value : -1 * value
    }
}

that used to work fine, but after this issue was closed, my file no longer compiles: "Invalid use of modifier static outside of a type declaration". What is the purpose of disallowing extensions from having static methods?

@fasterthanlime
Copy link
Collaborator

Simple oversight from @Shamanas's part I suppose?

Btw, we should have tests for this (with //! shouldfail - so that sam
checks that rock errs out)

On Tue, Aug 19, 2014 at 10:24 AM, davidhesselbom notifications@github.com
wrote:

I had a file like this:

import ./internal
import math

extend Float {
absolute: static func(value: This) -> This {
value >= 0 ? value : -1 * value
}
}

that used to work fine, but after this issue was close, my file no longer
compiles: "Invalid use of modifier static outside of a type declaration".
What is the purpose of disallowing extensions to have static methods?


Reply to this email directly or view it on GitHub
#797 (comment).

@alexnask
Copy link
Collaborator Author

It was not my intention to do that, I though that extentions where actually TypeDefs, it seems not, fixing + adding testcase.

And I did add tests. :P

@fasterthanlime
Copy link
Collaborator

But... extensions should allow static - just not 'abstract', for example

On Tue, Aug 19, 2014 at 10:32 AM, Alexandros Naskos <
notifications@github.com> wrote:

It was not my intention to do that, I though that extentions where
actually TypeDefs, it seems not, fixing + adding testcase.

And I did add tests. :P


Reply to this email directly or view it on GitHub
#797 (comment).

@fasterthanlime
Copy link
Collaborator

And I did add tests. :P

Well, I didn't have time to check it yet, but I wanted to mention the
undocumented sam feature

On Tue, Aug 19, 2014 at 10:35 AM, Amos Wenger fasterthanlime@gmail.com
wrote:

But... extensions should allow static - just not 'abstract', for example

On Tue, Aug 19, 2014 at 10:32 AM, Alexandros Naskos <
notifications@github.com> wrote:

It was not my intention to do that, I though that extentions where
actually TypeDefs, it seems not, fixing + adding testcase.

And I did add tests. :P


Reply to this email directly or view it on GitHub
#797 (comment)
.

@alexnask
Copy link
Collaborator Author

Ok, I have to make it a little more specific then.
I guess extentions should only disallow abstract?

@davidhesselbom
Copy link
Contributor

Maybe this is overly complicating things, but what if you want to create an extension for a type, and then inherit that type? Then extensions could have use for abstract. Otherwise, I guess it's not necessary.

@alexnask
Copy link
Collaborator Author

Don't quote me on that but I don't think it would work.
I believe extend-s (Addons) cannot modify the metaclass of the type they extend, so abstract methods won't work for them (it won't segfault or anything, the abstract method you are adding will not be inherited though)

@davidhesselbom
Copy link
Contributor

I guess that settles it then :)

@fasterthanlime
Copy link
Collaborator

The greek boy is correct. No class/inheritance for addons, they're just
sugar.

On Tue, Aug 19, 2014 at 11:06 AM, davidhesselbom notifications@github.com
wrote:

I guess that settles it then :)


Reply to this email directly or view it on GitHub
#797 (comment).

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

Successfully merging a pull request may close this issue.

3 participants