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

type-inference-independent comprehensions #7258

Closed
StefanKarpinski opened this issue Jun 13, 2014 · 65 comments
Closed

type-inference-independent comprehensions #7258

StefanKarpinski opened this issue Jun 13, 2014 · 65 comments
Assignees
Labels
breaking This change will break code needs decision A decision on this change is needed
Milestone

Comments

@StefanKarpinski
Copy link
Member

https://groups.google.com/forum/#!topic/julia-dev/DF8LUiH7FP4

I was looking for an issue for this but realized that it wasn't a GitHub issue yet. Now it is.

@stevengj
Copy link
Member

Why is this a breaking change?

@StefanKarpinski
Copy link
Member Author

Because it changes the type of comprehensions, which affects the behavior of programs.

@stevengj
Copy link
Member

My impression is that Jeff's algorithm will only produce tighter typing than we have now. Mostly this seems like it will affect performance, but I agree that in principle it could change other behaviors.

I would be suspicious about code whose correctness relies very sensitively on what type Julia infers for a comprehension, though, since Julia's type-inference abilities don't seem to be strictly documented at the moment. And changes in undocumented behaviors are a weaker type of "breaking" change.

@StefanKarpinski
Copy link
Member Author

I suspect this break a fair amount of code, actually. Comprehensions that currently return ByteString will return ASCIIString and the code will break when inserting UTF8Strings.

@StefanKarpinski
Copy link
Member Author

Just one example. There are many others.

@StefanKarpinski
Copy link
Member Author

Of course, maybe it's rarer than I think to insert things into the result of a comprehension.

@IainNZ
Copy link
Member

IainNZ commented Jun 14, 2014

I second @stevengj 's notion that it shouldn't matter - but I'll still be interested to see PackageEval results after its merged :)

@StefanKarpinski
Copy link
Member Author

Well, that would be a lovely surprise. Even though type inference behavior is undocumented, in a sense this is a much worse breaking change than API changes because you can't reasonably deprecate it.

@JeffBezanson
Copy link
Member

The good thing about this change is that it is not really possible to
"document" type inference behavior. Our inference necessarily involves
heuristics. Even in an ML-like language, where there is a straightforward
algorithm, what can be inferred is not always totally intuitive.
On Jun 14, 2014 12:04 AM, "Stefan Karpinski" [email protected]
wrote:

Well, that would be a lovely surprise. Even though type inference behavior
is undocumented, in a sense this is a much worse breaking change than API
changes because you can't reasonably deprecate it.


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

@StefanKarpinski
Copy link
Member Author

Agreed, getting rid of the dependence on these heuristics is why this is so appealing.

@kmsquire
Copy link
Member

I'm also really looking forward to this change.

In the referenced thread, Jeff suggested this might conflict with removing
automatic concatenation of [A,B], which I would REALLY like to see.

Is it possible to have my cake and ear it too here?

@kmsquire
Copy link
Member

Is it possible to have my cake and ear it too here?

ear -> 'ere. ;-)

@JeffBezanson
Copy link
Member

That has more to do with interpreting a comprehension as a "concatenation"
of a generator, which is separate from this issue.
On Jun 14, 2014 2:15 PM, "Kevin Squire" [email protected] wrote:

Is it possible to have my cake and ear it too here?

ear -> 'ere. ;-)


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

@ivarne
Copy link
Member

ivarne commented Jun 16, 2014

@IainNZ For potentially big breaking changes like this. Would it be possible to run PkgEvaluator on a branch, before we merge it into master?

@StefanKarpinski
Copy link
Member Author

Oh, man, being able to see the impact of a change across packages before merging it would be huge.

@joehuchette
Copy link
Member

FWIW Rust has a pretty cool approach to this kind of thing with bors, a bot that automatically takes PRs and tests them before they get merged into master: rust-lang/rust#14906

@StefanKarpinski
Copy link
Member Author

Cool. Maybe we should look into using that. Although @IainNZ's package evaluator is already pretty great.

@IainNZ
Copy link
Member

IainNZ commented Jun 16, 2014

Absolutely @ivarne

@joehuchette
Copy link
Member

It's really great, I was thinking more that this kind of thing could be a really slick front-end to PackageEvaluator that automatically spits results out onto Github.

@timholy
Copy link
Member

timholy commented Jun 16, 2014

@joehuchette, you know we already do that with Travis for Julia proper? Just open the page of pull requests, and look at the little green checkmarks or red xs.

This is specifically about running the tests in all the packages, too. I couldn't tell if bors does that?

@joehuchette
Copy link
Member

I believe Rust uses bors much as julia uses Travis: to run the test suite on PRs to see if anything breaks before merging. A bot in the vein of bors could offer a bit more granularity than Travis does (to the best of my knowledge): instead of running PackageEvaluator with every commit in a branch (and burying the results in a log), run it only when prompted by a collaborator and then dump the results directly to the PR conversation. There certainly is a fair amount of redundancy, though.

@IainNZ
Copy link
Member

IainNZ commented Jun 16, 2014

My first priority for a JuliaBot would actually be In METADATA, automatically apply the same logic PackageEvaluator does to the packages listed in the PR - no human intervention required there.
Something for PRs could be next. Having said that, the nightly runs have caught all the breaks within 24 hours of them occurring (equality, linalg, UTF16/32 strings), so its not clear if its really that high-impact an idea.

@pao
Copy link
Member

pao commented Jun 16, 2014

Having said that, the nightly runs have caught all the breaks within 24 hours of them occurring (equality, linalg, UTF16/32 strings), so its not clear if its really that high-impact an idea.

It might help to get an objective idea of how much pain a breaking change may cause, and when/whether to move forward with it.

@ivarne
Copy link
Member

ivarne commented Jun 16, 2014

It would make a great difference for Package authors to get notified a week before their package became incompatible with the latest master, rather than within 24 hours after. That will also draw attention to the change and Package authors might have something to add to the discussion.

@StefanKarpinski
Copy link
Member Author

All of these things. It would be a phenomenal way to minimize the pain of language changes.

@tkelman
Copy link
Contributor

tkelman commented May 5, 2016

Blocked on performance regressions at the moment? Still has a branch that could be rebased (significantly) for testing.

@StefanKarpinski
Copy link
Member Author

Yes, all of the blocking issues/prs are now merged, so we should revisit whether this can be done now.

@JeffBezanson
Copy link
Member

I think #15402 is the relevant regression here.

JeffBezanson added a commit that referenced this issue May 6, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue May 13, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue May 27, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue May 28, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 3, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
@JeffBezanson
Copy link
Member

Current PR: #16622
Blocked by: #16201
which is fixed by: #16692

@StefanKarpinski
Copy link
Member Author

Status update?

@JeffBezanson
Copy link
Member

Status is same as last week.

JeffBezanson added a commit that referenced this issue Jun 18, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 19, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 21, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 24, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 27, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jun 28, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jul 6, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
JeffBezanson added a commit that referenced this issue Jul 8, 2016
this removes `static_typeof` and `type_goto`

fixes #7258
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
this removes `static_typeof` and `type_goto`

fixes JuliaLang#7258
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests