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

Compiler: evaluate instance var initializers at the metaclass level #6414

Merged
merged 1 commit into from Jul 30, 2018

Conversation

Projects
None yet
6 participants
@asterite
Contributor

asterite commented Jul 20, 2018

Fixes #5876

This currently compiles:

class Foo
  @x : Int32 = bar

  def bar
    1
  end
end

However, it makes very little sense to run an instance var initializer at the instance level, because that's what we are initializing.

Java allows that behavior, but C# rejects it, demanding that bar has to be a static method. That makes much more sense to me.

In any case, you can initialize it at the instance level if you want to, in an initialize method:

class Foo
  @x : Int32

  def initialize
    @x = bar
  end

  def bar
    1
  end
end

That makes it much more clear that this is run at the instance level, and additionally the compiler will check that the instance var isn't used before it's assigned a value, etc.

It also makes sense from a scope level. If we do:

class Foo
  def bar
    1
  end

  bar # Error
  CONST = bar # error
  # but this works?: @x : Int32 = bar
end

So now it's uniform:

class Foo
  @x : Int32 = bar # ok
  bar # ok
  CONST = bar # ok

  def self.bar
    1
  end
end

This is a breaking change, but I doubt there's a lot of code out there that uses this kind of initializers.

@asterite asterite force-pushed the asterite:bug/5876-instance-var-initializer branch from 9ae5664 to c350121 Jul 20, 2018

@jhass

jhass approved these changes Jul 20, 2018

I want to say no, the current behavior makes total sense, but that's just me doing too much Java and ruby behaves in the proposed way, so 👍

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 20, 2018

In my opinion it seems logical that an ivar assignment behaves the same whether it is inside an initialize method or at class scope. I always figured these initializers as being in some kind of virtual initialize which is always called before the actual method. This can be particularly useful to make sure an ivar is always initialized even with subclass overrides of initialize. I'm not aware of a specific use case regarding instance methods, though.
I suppose the same sanity-checks could be applied to ivar initializers as inside an initialize method.

On the other hand, it's probably not that important and having a consistent rule is fine either way.

I just don't see an issue with CONST = bar and @ivar : Int32 = bar referring to different bar methods. The identifiers CONST and @ivar themselves are on different scopes, so it makes perfect sense that their value expression can be differently scoped as well.

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 20, 2018

Yeah, maybe it should run at the instance level. I'm not sure yet, so for now let's not merge this.

@Sija

This comment has been minimized.

Contributor

Sija commented Jul 20, 2018

ivar assignments are done at the class level so why should they be resolved differently? (see Ruby)

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 20, 2018

@Sija @straight-shoota What do you mean? There's no such thing in Ruby. When you do @x = 1 at the class level in Ruby, it assigns an instance variable of the class, not of the instance. In Crystal we want a different behavior for that same line.

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 20, 2018

@Sija Even if Ruby was different... In Crystal @x : Int32 declares a varibale in instance scope, so it would by no means be a surprise to anyone if the second part = bar also resolves in instance scope (it doesn't necessarily need be that way, type scope should be ok, too).

@Sija

This comment has been minimized.

Contributor

Sija commented Jul 20, 2018

@asterite @straight-shoota Sorry, my bad! I already forgot that in Ruby @ivars can be declared only inside of instance methods... Too much Crystal lately 😅 Still, with the issue at hand @x = bar declaration is happening inside of the class scope, so IMHO it intuitively should refer to to class rather than instance (you can always use it like that inside of initialize).

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Jul 20, 2018

And you can always reach class scope with self.class 😛

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 20, 2018

Actually, I tried to see if I can fix a few issues regarding initializers and instance vars... but it's all a big mess, in addition to how generics work or are implemented. I don't feel like fixing this by adding more patches and hacks. As usual, I believe it's better to have a good portion of time to rewrite the compiler from scratch, in a readable, efficient, well thought-out way (though at least I won't do that, ever, but if Crystal ever gets enough money to do it, I'd advise to do it).

@RX14

This comment has been minimized.

Member

RX14 commented Jul 21, 2018

@asterite I'm sure rewriting or refactoring portions of the compiler will be far more productive then throwing out the entire thing. If you're working on fixing semantics, then you probably dont want to be testing that with freshly written buggy codegen code.

Foo.new
),
"undefined local variable or method 'bar'"

This comment has been minimized.

@RX14

RX14 Jul 21, 2018

Member

can we detect this and emit a specialized "did you mean" error message? I suspect it wouldn't be easy but worth asking in case it is.

This comment has been minimized.

@asterite

asterite Jul 21, 2018

Contributor

I think it's possible, if a lookup fails, see if there's a method with that name in the class scope... but I don't feel like implement that right now (someone else can try it, though)

@RX14 RX14 added this to the 0.26.0 milestone Jul 21, 2018

@bcardiff

I think this change make sense, at least, today.

It would be great to eventually allow the declaration-initializers lines to be evaluated in context of the instance, but only when that is made in a sound way. Rules for typing, order, dependencies of values need to be considered.

With this change, initializers of constants values (literals, loggers, etc) still work.

So 👍 on my side. And maybe in the future if needed we can relax this a bit.

@asterite any final though given your comment

@asterite

This comment has been minimized.

Contributor

asterite commented Jul 30, 2018

I don't think there's a big pressure to merge this, or do something regarding this. It can be tackled in the future.

@RX14 RX14 merged commit 3de4c52 into crystal-lang:master Jul 30, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff referenced this pull request Aug 7, 2018

Closed

Update to crystal 0.26 #922

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