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

Implement .! in compiler #8445

Merged
merged 2 commits into from Nov 15, 2019
Merged

Conversation

@jan-zajic
Copy link
Contributor

jan-zajic commented Nov 5, 2019

Implements the boolean negation to be written as a regular method call.

I first see mentioned it here in #8327 where @straight-shoota documented it, but it was never implemented. Second time he mentioned it in issue #8383. Also compiler have warning '!' is a pseudo-method and can't be redefined but if you try used it, current stack is:

expecting any of these tokens: IDENT, CONST, +, -, *, /, //, %, |, &, ^, ~, **, <<, <, <=, ==, !=, =~, !~, >>, >, >=, <=>, ===, [], []=, []?, [, &+, &-, &*, &** (not '!') (Crystal::SyntaxException)
	 from src/compiler/crystal/syntax/lexer.cr:3001:9 in 'raise'
	 from src/compiler/crystal/syntax/lexer.cr:3000:5 in 'raise'
	 from src/compiler/crystal/syntax/parser.cr:267:11 in 'check'

ie it not pass even lexer and in parser it's not implemented.

@jan-zajic jan-zajic force-pushed the jan-zajic:feature/negation-suffix branch 3 times, most recently from fdfe133 to 78d3f16 Nov 5, 2019
@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 5, 2019

Hi!

Sorry, I don't understand what this does. Could you explain what you can do with this change? If it's about being able to define ! as a method, or redefine it, that's a no no

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 6, 2019

@asterite No. It's a missing language feature as now documented in src/docs_pseudo_methods.cr.

This method is a unary operator and usually written in prefix notation # (!foo) but it can also be written as a regular method call (foo.!).

Introduced in #8327 which you reviewed without comments. So if it's true this PR fix bug in compiler.
With this change you can basically do anyValue.!, but for details see a newly introduced negation_method_spec.cr.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 6, 2019

@asterite Currently you can't call the pseudo method ! using regular call syntax: foo.! is a syntax error. You can only use prefix notation: !foo.

This PR seems to fix that.

In the mentioned issues I was under the impression that this already works, as it does with any other pseudo method and unary operator. But it seems ! wasn't taken care of. So currently, the documentation for Object#! is incorrect. See also #8383 (comment)

It might not be super useful to write foo.! when you can do !foo but it's still important for consistency. And it allows using ! as block arg: foo &.!

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 6, 2019

Oooh... I see, I get it now. This is great, thank you!

However, this is just about parsing and expanding code. I think tests for parsing a d expanding code are needed, but semantic and codegen tests are not needed, like straight shoota mentions. That's why I was confused, I thought it changed more than parsing.

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 6, 2019

Ok, i can rework tests, i just don't known how they are structured and need them for implementing it.

@jan-zajic jan-zajic force-pushed the jan-zajic:feature/negation-suffix branch from 78d3f16 to 44fe2eb Nov 6, 2019
@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 6, 2019

@jan-zajic the specs should be in spec/compiler/normalize/not_spec.cr. In that directory, there are some of the specs regarding literal_expander. So expr.! is normalized to !expr.

The parsing of expr.! needs to be like a regular call. So the changes in the parser should allow that syntax and return Call instead of Not.

That should eliminate the need to handle when .! at a parser level.

@jan-zajic jan-zajic closed this Nov 6, 2019
@jan-zajic jan-zajic reopened this Nov 6, 2019
@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 6, 2019

So the changes in the parser should allow that syntax and return Call instead of Not

I'm not sure about this. Not is a special call and it's handled specially by the compiler. Doing it with Call won't do type flow stuff.

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 6, 2019

I came out of it that other pseudo-methods :is_a?, :as, :as?, :responds_to?, :nil? are currently implemented same, ie without call.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 6, 2019

Ok, my bad. Sorry for the noise.

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 6, 2019

@asterite, @bcardiff I added aditional tests to parser spec and to case expanding spec. I also simplify codegen specs. No one question remains: should i remove that whole negation_method_spec.cr in codegen? I'd prefer to keep it, to document current behavior and test if it works as all, not only parsing. I do not share your opinion it must works if parser and expanding works..

Semantically Not now can be on places where it wasn't before. The "order of words in sentences" can now be different. Also that codegen folder with real code examples is good place to look, when documentation for something is confusing/incomplete/missing and you need to really know, if something works that way and is not broken. Imho we can remove file spec/compiler/codegen/case_spec.cr as well, because case is transformed to if/elsif/else? As such it has no effect to code generation.

I think that codegen specs currently test if AST tree can produce working program and cover branches of semantic visitor that are not covered elsewhere. We can for example move that kind of tests to some different "integration" folder, but it's really that important? Is there some pressure on reducing the number of specs? I think coverage is more important.

But it's just my opinion, can just delete negation_method_spec.cr file and PR should be valid without it, if you make consensus.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 6, 2019

Semantically Not now can be on places where it wasn't before

Yes, but !1 produces the same ASTNode as 1.! so if codegen works for the first one (and there are already specs for that) then it will definitely work for the second one.

The only place where a spec might be needed is in when .!, though that again is just replacing an ImplicitObj with a Not and then it eventually leads to the same codegen, because it will boil down to an if.

Also, these specs, if we really want to keep them, should go into the existing codegen/not_spec.cr.

Imho we can remove file spec/compiler/codegen/case_spec.cr as well

I agree. It might be the case that case wasn't expanded in the past and there was specialized code for it, I don't know.

Is there some pressure on reducing the number of specs?

Yes, CI already takes a lot of time and for 32 bits builds we had to split specs to avoid exhausting the memory. More specs will make it more likely that this will happen again.

@jan-zajic jan-zajic force-pushed the jan-zajic:feature/negation-suffix branch from 44fe2eb to 1e00251 Nov 6, 2019
@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 6, 2019

@asterite OK, thank you for your time.

Signed-off-by: Jan Zajic <jan.zajic@corpus.cz>
@jan-zajic jan-zajic force-pushed the jan-zajic:feature/negation-suffix branch from 1e00251 to 4a71613 Nov 6, 2019
@jan-zajic jan-zajic requested a review from straight-shoota Nov 6, 2019
@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 8, 2019

@asterite please review this, thank you.

if @token.type == :"!"
obj = Var.new("self").at(location)
return parse_negation_suffix(obj)
end
Comment on lines 3931 to 3934

This comment has been minimized.

Copy link
@asterite

asterite Nov 8, 2019

Member

I don't think this is needed.

This would turn ! into !self, but you never want to do that.

In other cases plain is_a? it turned into self.is_a?, but for ! it doesn't make sense.

Maybe remove it and check that no parser spec is broken?

This comment has been minimized.

Copy link
@jan-zajic

jan-zajic Nov 8, 2019

Author Contributor

It's currently takes effect at parse_when_expression, it breaks if removed.
On other places parse_var_or_call is not called if token is :!. So i can leave it, or make extra condition in parse_when_expression which can be a little bit messy.

This comment has been minimized.

Copy link
@asterite

asterite Nov 8, 2019

Member

Ah, I see. Maybe add a comment saying that this will only trigger from parse_when_expression?

This comment has been minimized.

Copy link
@jan-zajic

jan-zajic Nov 8, 2019

Author Contributor

yes, added.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 8, 2019

Is there any reason darwin in CI takes aeons to finish? All other build steps finish relatively quickly.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 8, 2019

Is there any reason darwin in CI takes aeons to finish? All other build steps finish relatively quickly.

'cause there is a single queue for osx. And they need to wait a lot sometimes.

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 8, 2019

Is there any reason darwin in CI takes aeons to finish? All other build steps finish relatively quickly.

'cause there is a single queue for osx. And they need to wait a lot sometimes.

Maybe someone with permissions push that Rerurn button? if there is any chance at all it finish without timing out (same for test_preview_mt on almost all my PRs).

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Nov 8, 2019

test_preview_mt is expected to fail (I can't remember why)

@jan-zajic

This comment has been minimized.

Copy link
Contributor Author

jan-zajic commented Nov 15, 2019

So @asterite, can you approve this, or something else i can done?

@asterite asterite added this to the 0.32.0 milestone Nov 15, 2019
@asterite asterite merged commit 1580298 into crystal-lang:master Nov 15, 2019
4 of 6 checks passed
4 of 6 checks passed
ci/circleci: test_darwin Your tests failed on CircleCI
Details
ci/circleci: test_preview_mt Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.