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

Defer creation of hash maps into eval stage #1736

Merged
merged 1 commit into from
Jan 9, 2016

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Nov 14, 2015

This finally fixes #1169, which was caused by a severe bug with how we handle maps!
Additional Spec-Tests: sass/sass-spec#623

The current behavior seems rather random and I didn't care to check when exactly it wouldn't work as expected. The bug is rather obvious, as can be seen with the following example:

$map: (
  random(): foo,
  random(): bar
);

This will currently result in a duplicate key error. The parser actually already creates a map for the given sass code above, which will never work correctly, since we only know the actual keys after the evaluation phase (which is not available to the parser). It feels rather natural to use an even-sized list to carry it over to the evaluation phase, and create the actual map there.

A lot of languages allow the creation of maps from even-sized lists, so it felt natural to extend the existing List class to support this. But we only use this internally (from an external point, we only have space and comma separated lists). Unsure if sass can convert hash maps to lists and vice versa?

The error reporting is once again rather funky. It will always report the hex representation for duplicate color keys. This is the first time I see this happen and our codebase didn't support that in anyway without adding a new function just to get that representation (it's not to_string nor inspect).

ToDo:

  • Implement to_string for missing expression types
  • Re-use new to_string functions in CRTP visitors

@mgreter
Copy link
Contributor Author

mgreter commented Nov 24, 2015

I've implemented the missing details in #1754. The commit in here already fixes the most urgent bug, so I leave this one as is. With the interpolation refactoring and the one I've linked we're getting IMHO pretty close to a more sane and easier to use internal API.

Use an intermediate list representation to create
the actual hash map in the evaluation phase.
mgreter added a commit that referenced this pull request Jan 9, 2016
Defer creation of hash maps into eval stage
@mgreter mgreter merged commit 79a0e66 into sass:master Jan 9, 2016
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.

Quoted and unquoted colors in maps produces 'Duplicate key "transparent" in map'
1 participant