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

Bug: initialize with {% @type %} #6077

Merged
merged 2 commits into from May 8, 2018
Merged

Bug: initialize with {% @type %} #6077

merged 2 commits into from May 8, 2018

Conversation

asterite
Copy link
Member

@asterite asterite commented May 8, 2018

Fixes a bug found in #6063 (comment)

The compiler has a secret rule that's not documented (but probably should): if an initialize uses @type in macros, then checking what instance variables are initialized is delayed until the semantic phase, only for those methods. This allows the definition of initialize methods that are based on a type's instance vars, for example.

But there were a few bugs with that. This PR fixes those.

@asterite asterite self-assigned this May 8, 2018
@@ -4429,6 +4429,21 @@ describe "Semantic: instance var" do
), inject_primitives: false) { types["Foo"] }
end

it "is more permissive with macro def initialize, bug with named args" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference this PR number in the spec name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit recursive :-)

I usually reference the issue number (bug), but there's none here (well, there's that comment...). In any case, the blame of that line will lead to this commit, and in GitHub it will relate to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git blame can easilly be thrown off by a few refactors or file renames, far down the line.

@@ -108,7 +108,7 @@ struct Crystal::TypeDeclarationProcessor
# removed if an explicit type is found (in remove_error).
@errors = {} of Type => Hash(String, Error)

# Types that have a single macro def initialize
# Types whose initialize methods all are macro defs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/all are/are all/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@asterite asterite merged commit da46016 into crystal-lang:master May 8, 2018
@asterite asterite added this to the Next milestone May 8, 2018
@RX14 RX14 mentioned this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants