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

Initialize symbols before checking their annotations. #317

Merged
merged 1 commit into from Dec 29, 2017

Conversation

Projects
None yet
2 participants
@dragos
Contributor

dragos commented Dec 29, 2017

The Scala compiler uses laziness extensively to deal with recursive types. Annotations are one instance where an uninitialized type will be incorrect, so we call info before checking method annotations inside macros. The proper method, initialize, is not available in the public API so I just called .info, which is what most places in the compiler do, anyway.

Fix #172

Initialize symbols before checking their annotations.
The Scala compiler uses laziness extensively to deal with recursive types. Annotations are one instance where an uninitialized type will be incorrect, so we call `info` before checking method annotations inside macros. The proper method, initialize, is not available in the public API so I just called .info, which is what most places in the compiler do, anyway.

Fix #172
@OlegIlyenko

This comment has been minimized.

Member

OlegIlyenko commented Dec 29, 2017

This is a great insight, thanks a lot!

@OlegIlyenko OlegIlyenko merged commit c6c5c77 into sangria-graphql:master Dec 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dragos

This comment has been minimized.

Contributor

dragos commented Dec 30, 2017

Thanks for the quick turnaround! Do you have an estimate for the next release/a release containing this fix? One of our customers hit this issue and it would be nice to have a simple upgrade path.

@OlegIlyenko OlegIlyenko added this to the v1.3.4 milestone Dec 30, 2017

@OlegIlyenko

This comment has been minimized.

Member

OlegIlyenko commented Dec 30, 2017

I was thinking something like mid/end Jan, or beginning of Feb. There were quite a few changes to reference implementation, so I wanted to incorporate them into the next release after the rate of change stabilizes a bit (so I wanted to wait a bit). Some of these changes (I would say most important one) are already present in the recent v1.3.3.

Meanwhile, I can publish a snapshot version, if it would help. I also think that it should not be a big deal to publish a minor stable version with just this fix if you would consider this fix to be critical, though I normally trying to space out the releases so that they don't come too often. WDYT?

@dragos

This comment has been minimized.

Contributor

dragos commented Dec 31, 2017

Mid-January would be fine as a date, what I'm more concerned about is how easy it will be to upgrade given the other changes to go into 1.3.4. Are there any breaking changes?

In the meantime, maybe instead of a snapshot you could publish a milestone (a stable version number). If a bugfix release is easy to cut I'm happy to port my changes to the 1.3.4 tag (though I tend to agree that this is not a critical fix).

@OlegIlyenko

This comment has been minimized.

Member

OlegIlyenko commented Dec 31, 2017

Thanks for the info! v1.3.4 would be primarily focused on:

  • SDL-related syntax and semantic changes
  • A small validation overhaul (new validations + improvements to some existing one)

v1.3.3 already introduced some backward incompatible changes in SDL syntax. They are documented in #308. Since then we discussed it and addressed these concerns on a spec and reference implementation level. So one of the goals of v1.3.4 is to provide more backward compatibility in terms of SDL syntax with pre v1.3.3, I don't expect any major breaking changes.

Since it is not critical at the moment, I would suggest not to rush it for now (during holidays period) and proceed with original release plan.

dotta added a commit to dotta/sangria that referenced this pull request Jan 3, 2018

Merge pull request sangria-graphql#317 from dragos/issue/fix-172
Initialize symbols before checking their annotations.
(cherry picked from commit c6c5c77)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment