search

melt-umn / silver

An attribute grammar-based programming language for composable language extensions
http://melt.cs.umn.edu/silver/
GNU Lesser General Public License v3.0
59 stars 7 forks source link

core shadows imports #139

Open remexre opened 7 years ago

remexre commented 7 years ago

Eric ran into this recently; the new concat function in core shadows the one in langutil. This only occurs for imports, not import. Is this intended behavior? It looks like we've run up against this before, with all the imports silver:langutil:pp with implode as ppImplode; statements.

Also, is there a strong reason we have imports? It feels a little too "spooky action at a distance" for me...

tedinski commented 7 years ago

Oh heck, that patch really made a mess of things didn't it.

tedinski commented 7 years ago

I'm thinking about renaming concat in core to concatList just to make things easier. Thoughts?

ericvanwyk commented 7 years ago

Why don't we just report an error of using an ambiguously defined value? State that 'concat' exists in two imported libraries. Don't we do this for other ambiguously defined values?

On Thu, Apr 20, 2017 at 12:39 PM, Ted Kaminski notifications@github.com wrote:

I'm thinking about renaming concat in core to concatList just to make things easier. Thoughts?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/melt-umn/silver/issues/139#issuecomment-295828190, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhd5h82hsN1BTLEfqOLZqYQJdxFFCjks5rx5hCgaJpZM4NDIfK .

remexre commented 7 years ago

It looks like the opposite is happening; @krame505 suggested in ableC#36 that we just import-with the concat to ppConcat.

The good that's coming out of this is that it looks like we're finally setting up nightly builds of everything and getting notifications on Slack for them...

tedinski commented 7 years ago

I'm less concerned about the error message right now than I am concerned about the fact that I broke ablec, the tutorials, ablep, and the old 'simple' language in jenkins.

krame505 commented 7 years ago

We already started refactoring things to use ppConcat... It is probably just best to roll with it at this point and be consistent.

remexre commented 7 years ago

Also re: Jenkins; http://coldpress.cs.umn.edu:8080/job/silver/configure isn't loading?

ericvanwyk commented 7 years ago

If renaming 'concat' in core will fix things in the short term then that might be worth doing.

But longer term we should be sure that core doesn't shadow other imports.

On Thu, Apr 20, 2017 at 12:46 PM, Nathaniel Ringo notifications@github.com wrote:

Also re: Jenkins; http://coldpress.cs.umn.edu:8080/job/silver/configure isn't loading?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/silver/issues/139#issuecomment-295830485, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhdwSiloK__2XPY-mAxYhZBSI0y1_1ks5rx5oHgaJpZM4NDIfK .

tedinski commented 7 years ago

RE: the actual issue reported in the OP: It's a mess because Silver's importing mechanism wasn't really designed so much as it accreted.

🎵 In the beginning 🎵 you imported core manually. Somewhere along the line, this became implicit and optional. Then imports was added as a grammar-wide facility. But there might still be import core in individual files. So instead of anything sensible we have all the imports as one scope, followed by all the file's import as another scope. And core gets stuffed into the file's import scope automatically if it's not there already. Somewhere, I forget exactly, there's another scope with all the grammar's own symbols. It's a mess.

tedinski commented 7 years ago

@remexre It seems Jenkins is busted. I'm going to go try to fix it now.

remexre commented 7 years ago

So, to solve this (correct me if I'm wrong):

and long-term, redesign importing?

I continue to be skeptical that imports should be a thing...

ericvanwyk commented 7 years ago

Ok, so maybe this is a longer term fix then.

So, very short term is rename 'concat' in core.

Intermediate term: can we generate a better error message?

Long term: fix the imports mechanism.

On Thu, Apr 20, 2017 at 12:54 PM, Ted Kaminski notifications@github.com wrote:

@remexre https://github.com/remexre It seems Jenkins is busted. I'm going to go try to fix it now.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/silver/issues/139#issuecomment-295833532, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhdxD6wgqMi67i5WqRlRoPvaFgtoIPks5rx5vygaJpZM4NDIfK .

remexre commented 7 years ago

I think we'd want to keep both named concat though; I'd be more in favor of fixing the import order, or import-with-ing if that'd be an invasive change.

krame505 commented 7 years ago

OK, so then to be clear, you want us to change this back in ableC?

krame505 commented 7 years ago

Yes, I am with @remexre on this

krame505 commented 7 years ago

How bad would it be comparatively to just fix the imports in the various other projects?

remexre commented 7 years ago

I did it with sed command; we could just run:

find . -name '*.sv' | xargs sed -i 's/concat/ppConcat/g'
find . -name '*.sv' | xargs sed -i 's/imports silver:langutil:pp with implode as ppImplode;/imports silver:langutil:pp with implode as ppImplode, concat as ppConcat;/'
tedinski commented 7 years ago

Well, someone can sign up to do a similar fix on the tutorials in Silver.

And someone can do a similar fix on the 'simple' grammar in melt-svn.

We might have an issue with ableP because Jenkins might be building this from SVN, including using the SVN version of ableC though.

krame505 commented 7 years ago

How hard would it be to just have core move to the imports scope instead of import?

tedinski commented 7 years ago

Somewhat hard, because of the possibility that some files directly import core. Recall also that Silver doesn't cope well with multiple imports of the same grammar.

ericvanwyk commented 7 years ago

This would seem like a good thing to do.

On Thu, Apr 20, 2017 at 1:14 PM, Lucas Kramer notifications@github.com wrote:

How hard would it be to just have core move to the imports scope instead of import?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/melt-umn/silver/issues/139#issuecomment-295841193, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOhdxqQxM3QMMy-DzWpHOBq8r5S9tBTks5rx6CBgaJpZM4NDIfK .

krame505 commented 7 years ago

Then let's go ahead and fix those imports. This shouldn't be too hard to do with sed. Eric said that he is OK with letting ableP go for now, and someday maybe move it to github and get it working again.

tedinski commented 7 years ago

By fix you mean add imports ... with concat as ppConcat you mean?

krame505 commented 7 years ago

Yes, and delete places where we explicitly import core;

tedinski commented 7 years ago

I'm in favor of removing all legacy instances of import core but I think it's necessary in some cases. silver:langutil:pp itself does import core hiding implode. It's possible it isn't necessary anymore for some reason... want to find out? :)

tedinski commented 6 years ago

So anyway, I revisited this root problem a bit.

We decide to import core conditionally, based on whether it's mentioned explicitly in the grammar itself. I think we might be able to change this to include core unconditionally. But we could also make core its own outer scope regardless.

Right now our scopes are [imports with s + core maybe, import, the grammar itself] in order from outermost to innermost global scope. We could try out [core, imports, import, grammar] and see if it breaks anything.